[GIT PULL 00/12] first batch of EFI changes for v4.17

[GIT PULL 00/12] first batch of EFI changes for v4.17

[Ard Biesheuvel]

The following changes since commit 7928b2cbe55b2a410a0f5c1f154610059c57b1b2:

  Linux 4.16-rc1 (2018-02-11 15:04:29 -0800)

are available in the Git repository at:

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

for you to fetch changes up to 67a2152c7362bd9ee3ba6d218e435a04c27c32aa:

  efi: make const array 'apple' static (2018-03-08 07:54:10 +0000)

----------------------------------------------------------------
First batch of EFI changes for v4.17:
- use efi_switch_mm() on x86 instead of manipulating %cr3 directly (Sai)
- some fixes for the apple-properties code (Andy)
- add WARN() on arm64 if UEFI Runtime Services corrupt the reserved x18
  register (Ard)
- other minor cleanups and bugfixes

----------------------------------------------------------------
Andy Shevchenko (2):
      efi/apple-properties: Device core takes care of empty properties
      efi/apple-properties: Use memremap() instead of ioremap()

Ard Biesheuvel (3):
      efi/arm*: Stop printing addresses of virtual mappings
      efi: arm64: Check whether x18 is preserved by runtime services calls
      efi: reorder pr_notice() with add_device_randomness() call

Colin Ian King (1):
      efi: make const array 'apple' static

Jia-Ju Bai (1):
      x86: efi: Replace GFP_ATOMIC with GFP_KERNEL in efi_query_variable_store

Luis de Bethencourt (1):
      efi/x86: Fix trailing semicolons

Mark Rutland (1):
      efi/arm*: Only register page tables when they exist

Sai Praneeth (3):
      efi: Use efi_mm in x86 as well as ARM
      x86/efi: Replace efi_pgd with efi_mm.pgd
      x86/efi: Use efi_switch_mm() rather than manually twiddling with %cr3

 arch/arm64/include/asm/efi.h            |  4 ++-
 arch/arm64/kernel/Makefile              |  3 +-
 arch/arm64/kernel/efi-rt-wrapper.S      | 41 +++++++++++++++++++++++
 arch/arm64/kernel/efi.c                 |  6 ++++
 arch/x86/boot/compressed/eboot.c        |  6 ++--
 arch/x86/include/asm/efi.h              | 29 ++++++++---------
 arch/x86/platform/efi/efi_64.c          | 58 ++++++++++++++++++---------------
 arch/x86/platform/efi/efi_thunk_64.S    |  2 +-
 arch/x86/platform/efi/quirks.c          |  2 +-
 drivers/firmware/efi/apple-properties.c | 20 ++++--------
 drivers/firmware/efi/arm-runtime.c      | 17 +++-------
 drivers/firmware/efi/efi.c              | 11 ++++++-
 include/linux/efi.h                     |  2 ++
 13 files changed, 125 insertions(+), 76 deletions(-)
 create mode 100644 arch/arm64/kernel/efi-rt-wrapper.S

[PATCH 01/12] efi/arm*: Only register page tables when they exist

[Ard Biesheuvel]

From: Mark Rutland <mark.rutland@arm.com>

Currently the arm/arm64 runtime code registers the runtime servies
pagetables with ptdump regardless of whether runtime services page
tables have been created.

As efi_mm.pgd is NULL in these cases, attempting to dump the efi page
tables results in a NULL pointer dereference in the ptdump code:

/sys/kernel/debug# cat efi_page_tables
[  479.522600] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[  479.522715] Mem abort info:
[  479.522764]   ESR = 0x96000006
[  479.522850]   Exception class = DABT (current EL), IL = 32 bits
[  479.522899]   SET = 0, FnV = 0
[  479.522937]   EA = 0, S1PTW = 0
[  479.528200] Data abort info:
[  479.528230]   ISV = 0, ISS = 0x00000006
[  479.528317]   CM = 0, WnR = 0
[  479.528317] user pgtable: 4k pages, 48-bit VAs, pgd = 0000000064ab0cb0
[  479.528449] [0000000000000000] *pgd=00000000fbbe4003, *pud=00000000fb66e003, *pmd=0000000000000000
[  479.528600] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[  479.528664] Modules linked in:
[  479.528699] CPU: 0 PID: 2457 Comm: cat Not tainted 4.15.0-rc3-00065-g2ad2ee7ecb5c-dirty #7
[  479.528799] Hardware name: FVP Base (DT)
[  479.528899] pstate: 00400009 (nzcv daif +PAN -UAO)
[  479.528941] pc : walk_pgd.isra.1+0x20/0x1d0
[  479.529011] lr : ptdump_walk_pgd+0x30/0x50
[  479.529105] sp : ffff00000bf4bc20
[  479.529185] x29: ffff00000bf4bc20 x28: 0000ffff9d22e000
[  479.529271] x27: 0000000000020000 x26: ffff80007b4c63c0
[  479.529358] x25: 00000000014000c0 x24: ffff80007c098900
[  479.529445] x23: ffff00000bf4beb8 x22: 0000000000000000
[  479.529532] x21: ffff00000bf4bd70 x20: 0000000000000001
[  479.529618] x19: ffff00000bf4bcb0 x18: 0000000000000000
[  479.529760] x17: 000000000041a1c8 x16: ffff0000082139d8
[  479.529800] x15: 0000ffff9d3c6030 x14: 0000ffff9d2527f4
[  479.529924] x13: 00000000000003f3 x12: 0000000000000038
[  479.530000] x11: 0000000000000003 x10: 0101010101010101
[  479.530099] x9 : 0000000017e94050 x8 : 000000000000003f
[  479.530226] x7 : 0000000000000000 x6 : 0000000000000000
[  479.530313] x5 : 0000000000000001 x4 : 0000000000000000
[  479.530416] x3 : ffff000009069fd8 x2 : 0000000000000000
[  479.530500] x1 : 0000000000000000 x0 : 0000000000000000
[  479.530599] Process cat (pid: 2457, stack limit = 0x000000005d1b0e6f)
[  479.530660] Call trace:
[  479.530746]  walk_pgd.isra.1+0x20/0x1d0
[  479.530833]  ptdump_walk_pgd+0x30/0x50
[  479.530907]  ptdump_show+0x10/0x20
[  479.530920]  seq_read+0xc8/0x470
[  479.531023]  full_proxy_read+0x60/0x90
[  479.531100]  __vfs_read+0x18/0x100
[  479.531180]  vfs_read+0x88/0x160
[  479.531267]  SyS_read+0x48/0xb0
[  479.531299]  el0_svc_naked+0x20/0x24
[  479.531400] Code: 91400420 f90033a0 a90707a2 f9403fa0 (f9400000)
[  479.531499] ---[ end trace bfe8e28d8acb2b67 ]---
Segmentation fault

Let's avoid this problem by only registering the tables after their
successful creation, which is also less confusing when EFI runtime
services are not in use.

Reported-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/arm-runtime.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 1cc41c3d6315..86a1ad17a32e 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -54,6 +54,9 @@ static struct ptdump_info efi_ptdump_info = {
 
 static int __init ptdump_init(void)
 {
+	if (!efi_enabled(EFI_RUNTIME_SERVICES))
+		return 0;
+
 	return ptdump_debugfs_register(&efi_ptdump_info, "efi_page_tables");
 }
 device_initcall(ptdump_init);
-- 
2.15.1

[tip:efi/core] efi/arm*: Only register page tables when they exist

[tip-bot for Mark Rutland]

Commit-ID:  6b31a2fa1e8f7bc6c2a474b4a12dad7a145cf83d
Gitweb:     https://git.kernel.org/tip/6b31a2fa1e8f7bc6c2a474b4a12dad7a145cf83d
Author:     Mark Rutland <mark.rutland@arm.com>
AuthorDate: Thu, 8 Mar 2018 08:00:09 +0000
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 9 Mar 2018 08:58:21 +0100

efi/arm*: Only register page tables when they exist

Currently the arm/arm64 runtime code registers the runtime servies
pagetables with ptdump regardless of whether runtime services page
tables have been created.

As efi_mm.pgd is NULL in these cases, attempting to dump the efi page
tables results in a NULL pointer dereference in the ptdump code:

/sys/kernel/debug# cat efi_page_tables
[  479.522600] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[  479.522715] Mem abort info:
[  479.522764]   ESR = 0x96000006
[  479.522850]   Exception class = DABT (current EL), IL = 32 bits
[  479.522899]   SET = 0, FnV = 0
[  479.522937]   EA = 0, S1PTW = 0
[  479.528200] Data abort info:
[  479.528230]   ISV = 0, ISS = 0x00000006
[  479.528317]   CM = 0, WnR = 0
[  479.528317] user pgtable: 4k pages, 48-bit VAs, pgd = 0000000064ab0cb0
[  479.528449] [0000000000000000] *pgd=00000000fbbe4003, *pud=00000000fb66e003, *pmd=0000000000000000
[  479.528600] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[  479.528664] Modules linked in:
[  479.528699] CPU: 0 PID: 2457 Comm: cat Not tainted 4.15.0-rc3-00065-g2ad2ee7ecb5c-dirty #7
[  479.528799] Hardware name: FVP Base (DT)
[  479.528899] pstate: 00400009 (nzcv daif +PAN -UAO)
[  479.528941] pc : walk_pgd.isra.1+0x20/0x1d0
[  479.529011] lr : ptdump_walk_pgd+0x30/0x50
[  479.529105] sp : ffff00000bf4bc20
[  479.529185] x29: ffff00000bf4bc20 x28: 0000ffff9d22e000
[  479.529271] x27: 0000000000020000 x26: ffff80007b4c63c0
[  479.529358] x25: 00000000014000c0 x24: ffff80007c098900
[  479.529445] x23: ffff00000bf4beb8 x22: 0000000000000000
[  479.529532] x21: ffff00000bf4bd70 x20: 0000000000000001
[  479.529618] x19: ffff00000bf4bcb0 x18: 0000000000000000
[  479.529760] x17: 000000000041a1c8 x16: ffff0000082139d8
[  479.529800] x15: 0000ffff9d3c6030 x14: 0000ffff9d2527f4
[  479.529924] x13: 00000000000003f3 x12: 0000000000000038
[  479.530000] x11: 0000000000000003 x10: 0101010101010101
[  479.530099] x9 : 0000000017e94050 x8 : 000000000000003f
[  479.530226] x7 : 0000000000000000 x6 : 0000000000000000
[  479.530313] x5 : 0000000000000001 x4 : 0000000000000000
[  479.530416] x3 : ffff000009069fd8 x2 : 0000000000000000
[  479.530500] x1 : 0000000000000000 x0 : 0000000000000000
[  479.530599] Process cat (pid: 2457, stack limit = 0x000000005d1b0e6f)
[  479.530660] Call trace:
[  479.530746]  walk_pgd.isra.1+0x20/0x1d0
[  479.530833]  ptdump_walk_pgd+0x30/0x50
[  479.530907]  ptdump_show+0x10/0x20
[  479.530920]  seq_read+0xc8/0x470
[  479.531023]  full_proxy_read+0x60/0x90
[  479.531100]  __vfs_read+0x18/0x100
[  479.531180]  vfs_read+0x88/0x160
[  479.531267]  SyS_read+0x48/0xb0
[  479.531299]  el0_svc_naked+0x20/0x24
[  479.531400] Code: 91400420 f90033a0 a90707a2 f9403fa0 (f9400000)
[  479.531499] ---[ end trace bfe8e28d8acb2b67 ]---
Segmentation fault

Let's avoid this problem by only registering the tables after their
successful creation, which is also less confusing when EFI runtime
services are not in use.

Reported-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Acked-by: Will Deacon <will.deacon@arm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-efi@vger.kernel.org
Link: http://lkml.kernel.org/r/20180308080020.22828-2-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/firmware/efi/arm-runtime.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 1cc41c3d6315..86a1ad17a32e 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -54,6 +54,9 @@ static struct ptdump_info efi_ptdump_info = {
 
 static int __init ptdump_init(void)
 {
+	if (!efi_enabled(EFI_RUNTIME_SERVICES))
+		return 0;
+
 	return ptdump_debugfs_register(&efi_ptdump_info, "efi_page_tables");
 }
 device_initcall(ptdump_init);

[PATCH 02/12] efi/apple-properties: Device core takes care of empty properties

[Ard Biesheuvel]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

There is no need to artificially supply a property length and fake data
if property has type of boolean.

Remove redundant piece of data and code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-and-tested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/apple-properties.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/firmware/efi/apple-properties.c b/drivers/firmware/efi/apple-properties.c
index 9f6bcf173b0e..b9602e0d7b50 100644
--- a/drivers/firmware/efi/apple-properties.c
+++ b/drivers/firmware/efi/apple-properties.c
@@ -52,8 +52,6 @@ struct properties_header {
 	struct dev_header dev_header[0];
 };
 
-static u8 one __initdata = 1;
-
 static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
 					     struct device *dev, void *ptr,
 					     struct property_entry entry[])
@@ -95,14 +93,9 @@ static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
 			     key_len - sizeof(key_len));
 
 		entry[i].name = key;
-		entry[i].is_array = true;
 		entry[i].length = val_len - sizeof(val_len);
+		entry[i].is_array = !!entry[i].length;
 		entry[i].pointer.raw_data = ptr + key_len + sizeof(val_len);
-		if (!entry[i].length) {
-			/* driver core doesn't accept empty properties */
-			entry[i].length = 1;
-			entry[i].pointer.raw_data = &one;
-		}
 
 		if (dump_properties) {
 			dev_info(dev, "property: %s\n", entry[i].name);
-- 
2.15.1

[tip:efi/core] efi/apple-properties: Remove redundant attribute initialization from unmarshal_key_value_pairs()

[tip-bot for Andy Shevchenko]

Commit-ID:  6e98503dba64e721ba839e75dca036266ec0140f
Gitweb:     https://git.kernel.org/tip/6e98503dba64e721ba839e75dca036266ec0140f
Author:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
AuthorDate: Thu, 8 Mar 2018 08:00:10 +0000
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 9 Mar 2018 08:58:21 +0100

efi/apple-properties: Remove redundant attribute initialization from unmarshal_key_value_pairs()

There is no need to artificially supply a property length and fake data
if property has type of boolean.

Remove redundant piece of data and code.

Reviewed-and-tested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-efi@vger.kernel.org
Link: http://lkml.kernel.org/r/20180308080020.22828-3-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/firmware/efi/apple-properties.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/firmware/efi/apple-properties.c b/drivers/firmware/efi/apple-properties.c
index 9f6bcf173b0e..b9602e0d7b50 100644
--- a/drivers/firmware/efi/apple-properties.c
+++ b/drivers/firmware/efi/apple-properties.c
@@ -52,8 +52,6 @@ struct properties_header {
 	struct dev_header dev_header[0];
 };
 
-static u8 one __initdata = 1;
-
 static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
 					     struct device *dev, void *ptr,
 					     struct property_entry entry[])
@@ -95,14 +93,9 @@ static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
 			     key_len - sizeof(key_len));
 
 		entry[i].name = key;
-		entry[i].is_array = true;
 		entry[i].length = val_len - sizeof(val_len);
+		entry[i].is_array = !!entry[i].length;
 		entry[i].pointer.raw_data = ptr + key_len + sizeof(val_len);
-		if (!entry[i].length) {
-			/* driver core doesn't accept empty properties */
-			entry[i].length = 1;
-			entry[i].pointer.raw_data = &one;
-		}
 
 		if (dump_properties) {
 			dev_info(dev, "property: %s\n", entry[i].name);

[PATCH 03/12] efi/arm*: Stop printing addresses of virtual mappings

[Ard Biesheuvel]

With the recent %p -> %px changes, we now get something like this in
the kernel boot log on ARM/arm64 EFI systems:

     Remapping and enabling EFI services.
       EFI remap 0x00000087fb830000 =>         (ptrval)
       EFI remap 0x00000087fbdb0000 =>         (ptrval)
       EFI remap 0x00000087fffc0000 =>         (ptrval)

The physical addresses of the UEFI runtime regions will also be
printed when booting with the efi=debug command line option, and the
virtual addresses can be inspected via /sys/kernel/debug/efi_page_tables
(if enabled). So let's just remove the lines above.

Acked-by: Leif Lindholm <leif.lindholm@linaro.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/arm-runtime.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 86a1ad17a32e..13561aeb7396 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -83,10 +83,7 @@ static bool __init efi_virtmap_init(void)
 			return false;
 
 		ret = efi_create_mapping(&efi_mm, md);
-		if  (!ret) {
-			pr_info("  EFI remap %pa => %p\n",
-				&phys, (void *)(unsigned long)md->virt_addr);
-		} else {
+		if (ret) {
 			pr_warn("  EFI remap %pa: failed to create mapping (%d)\n",
 				&phys, ret);
 			return false;
-- 
2.15.1

[tip:efi/core] efi/arm*: Stop printing addresses of virtual mappings

[tip-bot for Ard Biesheuvel]

Commit-ID:  1832e64162ffbbbdf7230401298550f2b624351b
Gitweb:     https://git.kernel.org/tip/1832e64162ffbbbdf7230401298550f2b624351b
Author:     Ard Biesheuvel <ard.biesheuvel@linaro.org>
AuthorDate: Thu, 8 Mar 2018 08:00:11 +0000
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 9 Mar 2018 08:58:22 +0100

efi/arm*: Stop printing addresses of virtual mappings

With the recent %p -> %px changes, we now get something like this in
the kernel boot log on ARM/arm64 EFI systems:

     Remapping and enabling EFI services.
       EFI remap 0x00000087fb830000 =>         (ptrval)
       EFI remap 0x00000087fbdb0000 =>         (ptrval)
       EFI remap 0x00000087fffc0000 =>         (ptrval)

The physical addresses of the UEFI runtime regions will also be
printed when booting with the efi=debug command line option, and the
virtual addresses can be inspected via /sys/kernel/debug/efi_page_tables
(if enabled).

So let's just remove the lines above.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Acked-by: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-efi@vger.kernel.org
Link: http://lkml.kernel.org/r/20180308080020.22828-4-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/firmware/efi/arm-runtime.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 86a1ad17a32e..13561aeb7396 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -83,10 +83,7 @@ static bool __init efi_virtmap_init(void)
 			return false;
 
 		ret = efi_create_mapping(&efi_mm, md);
-		if  (!ret) {
-			pr_info("  EFI remap %pa => %p\n",
-				&phys, (void *)(unsigned long)md->virt_addr);
-		} else {
+		if (ret) {
 			pr_warn("  EFI remap %pa: failed to create mapping (%d)\n",
 				&phys, ret);
 			return false;

[PATCH 04/12] efi/x86: Fix trailing semicolons

[Ard Biesheuvel]

From: Luis de Bethencourt <luisbg@kernel.org>

The trailing semicolon is an empty statement that does no operation.
Removing them since they don't do anything.

Signed-off-by: Luis de Bethencourt <luisbg@kernel.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/x86/boot/compressed/eboot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 353e20c3f114..886a9115af62 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -439,7 +439,7 @@ setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height)
 	struct efi_uga_draw_protocol *uga = NULL, *first_uga;
 	efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
 	unsigned long nr_ugas;
-	u32 *handles = (u32 *)uga_handle;;
+	u32 *handles = (u32 *)uga_handle;
 	efi_status_t status = EFI_INVALID_PARAMETER;
 	int i;
 
@@ -484,7 +484,7 @@ setup_uga64(void **uga_handle, unsigned long size, u32 *width, u32 *height)
 	struct efi_uga_draw_protocol *uga = NULL, *first_uga;
 	efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
 	unsigned long nr_ugas;
-	u64 *handles = (u64 *)uga_handle;;
+	u64 *handles = (u64 *)uga_handle;
 	efi_status_t status = EFI_INVALID_PARAMETER;
 	int i;
 
-- 
2.15.1

[PATCH 05/12] efi: arm64: Check whether x18 is preserved by runtime services calls

[Ard Biesheuvel]

Whether or not we will ever decide to start using x18 as a platform
register in Linux is uncertain, but by that time, we will need to
ensure that UEFI runtime services calls don't corrupt it. So let's
start issuing warnings now for this, and increase the likelihood that
these firmware images have all been replaced by that time.

This has been fixed on the EDK2 side in commit 6d73863b5464
("BaseTools/tools_def AARCH64: mark register x18 as reserved").,
dated July 13, 2017.

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/efi.h       |  4 +++-
 arch/arm64/kernel/Makefile         |  3 ++-
 arch/arm64/kernel/efi-rt-wrapper.S | 41 ++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/efi.c            |  6 ++++++
 4 files changed, 52 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm64/kernel/efi-rt-wrapper.S

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8389050328bb..192d791f1103 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -31,7 +31,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
 ({									\
 	efi_##f##_t *__f;						\
 	__f = p->f;							\
-	__f(args);							\
+	__efi_rt_asm_wrapper(__f, #f, args);				\
 })
 
 #define arch_efi_call_virt_teardown()					\
@@ -40,6 +40,8 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
 	efi_virtmap_unload();						\
 })
 
+efi_status_t __efi_rt_asm_wrapper(void *, const char *, ...);
+
 #define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
 
 /* arch specific definitions used by the stub code */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index b87541360f43..6a4bd80c75bd 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -38,7 +38,8 @@ arm64-obj-$(CONFIG_CPU_PM)		+= sleep.o suspend.o
 arm64-obj-$(CONFIG_CPU_IDLE)		+= cpuidle.o
 arm64-obj-$(CONFIG_JUMP_LABEL)		+= jump_label.o
 arm64-obj-$(CONFIG_KGDB)		+= kgdb.o
-arm64-obj-$(CONFIG_EFI)			+= efi.o efi-entry.stub.o
+arm64-obj-$(CONFIG_EFI)			+= efi.o efi-entry.stub.o		\
+					   efi-rt-wrapper.o
 arm64-obj-$(CONFIG_PCI)			+= pci.o
 arm64-obj-$(CONFIG_ARMV8_DEPRECATED)	+= armv8_deprecated.o
 arm64-obj-$(CONFIG_ACPI)		+= acpi.o
diff --git a/arch/arm64/kernel/efi-rt-wrapper.S b/arch/arm64/kernel/efi-rt-wrapper.S
new file mode 100644
index 000000000000..05235ebb336d
--- /dev/null
+++ b/arch/arm64/kernel/efi-rt-wrapper.S
@@ -0,0 +1,41 @@
+/*
+ * Copyright (C) 2018 Linaro Ltd <ard.biesheuvel@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/linkage.h>
+
+ENTRY(__efi_rt_asm_wrapper)
+	stp	x29, x30, [sp, #-32]!
+	mov	x29, sp
+
+	/*
+	 * Register x18 is designated as the 'platform' register by the AAPCS,
+	 * which means firmware running at the same exception level as the OS
+	 * (such as UEFI) should never touch it.
+	 */
+	stp	x1, x18, [sp, #16]
+
+	/*
+	 * We are lucky enough that no EFI runtime services take more than
+	 * 5 arguments, so all are passed in registers rather than via the
+	 * stack.
+	 */
+	mov	x8, x0
+	mov	x0, x2
+	mov	x1, x3
+	mov	x2, x4
+	mov	x3, x5
+	mov	x4, x6
+	blr	x8
+
+	ldp	x1, x2, [sp, #16]
+	cmp	x2, x18
+	ldp	x29, x30, [sp], #32
+	b.ne	0f
+	ret
+0:	b	efi_handle_corrupted_x18	// tail call
+ENDPROC(__efi_rt_asm_wrapper)
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index f85ac58d08a3..fb5b3cd3a1c7 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -126,3 +126,9 @@ bool efi_poweroff_required(void)
 {
 	return efi_enabled(EFI_RUNTIME_SERVICES);
 }
+
+asmlinkage efi_status_t efi_handle_corrupted_x18(efi_status_t s, const char *f)
+{
+	pr_err_ratelimited(FW_BUG "register x18 corrupted by EFI %s\n", f);
+	return s;
+}
-- 
2.15.1

[tip:efi/core] efi/arm64: Check whether x18 is preserved by runtime services calls

[tip-bot for Ard Biesheuvel]

Commit-ID:  7e611e7dbb235938fca1dd359bad5e5f86ceabcb
Gitweb:     https://git.kernel.org/tip/7e611e7dbb235938fca1dd359bad5e5f86ceabcb
Author:     Ard Biesheuvel <ard.biesheuvel@linaro.org>
AuthorDate: Thu, 8 Mar 2018 08:00:13 +0000
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 9 Mar 2018 08:58:22 +0100

efi/arm64: Check whether x18 is preserved by runtime services calls

Whether or not we will ever decide to start using x18 as a platform
register in Linux is uncertain, but by that time, we will need to
ensure that UEFI runtime services calls don't corrupt it.

So let's start issuing warnings now for this, and increase the
likelihood that these firmware images have all been replaced by that time.

This has been fixed on the EDK2 side in commit:

  6d73863b5464 ("BaseTools/tools_def AARCH64: mark register x18 as reserved")

dated July 13, 2017.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Acked-by: Will Deacon <will.deacon@arm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-efi@vger.kernel.org
Link: http://lkml.kernel.org/r/20180308080020.22828-6-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/arm64/include/asm/efi.h       |  4 +++-
 arch/arm64/kernel/Makefile         |  3 ++-
 arch/arm64/kernel/efi-rt-wrapper.S | 41 ++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/efi.c            |  6 ++++++
 4 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8389050328bb..192d791f1103 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -31,7 +31,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
 ({									\
 	efi_##f##_t *__f;						\
 	__f = p->f;							\
-	__f(args);							\
+	__efi_rt_asm_wrapper(__f, #f, args);				\
 })
 
 #define arch_efi_call_virt_teardown()					\
@@ -40,6 +40,8 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
 	efi_virtmap_unload();						\
 })
 
+efi_status_t __efi_rt_asm_wrapper(void *, const char *, ...);
+
 #define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
 
 /* arch specific definitions used by the stub code */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index b87541360f43..6a4bd80c75bd 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -38,7 +38,8 @@ arm64-obj-$(CONFIG_CPU_PM)		+= sleep.o suspend.o
 arm64-obj-$(CONFIG_CPU_IDLE)		+= cpuidle.o
 arm64-obj-$(CONFIG_JUMP_LABEL)		+= jump_label.o
 arm64-obj-$(CONFIG_KGDB)		+= kgdb.o
-arm64-obj-$(CONFIG_EFI)			+= efi.o efi-entry.stub.o
+arm64-obj-$(CONFIG_EFI)			+= efi.o efi-entry.stub.o		\
+					   efi-rt-wrapper.o
 arm64-obj-$(CONFIG_PCI)			+= pci.o
 arm64-obj-$(CONFIG_ARMV8_DEPRECATED)	+= armv8_deprecated.o
 arm64-obj-$(CONFIG_ACPI)		+= acpi.o
diff --git a/arch/arm64/kernel/efi-rt-wrapper.S b/arch/arm64/kernel/efi-rt-wrapper.S
new file mode 100644
index 000000000000..05235ebb336d
--- /dev/null
+++ b/arch/arm64/kernel/efi-rt-wrapper.S
@@ -0,0 +1,41 @@
+/*
+ * Copyright (C) 2018 Linaro Ltd <ard.biesheuvel@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/linkage.h>
+
+ENTRY(__efi_rt_asm_wrapper)
+	stp	x29, x30, [sp, #-32]!
+	mov	x29, sp
+
+	/*
+	 * Register x18 is designated as the 'platform' register by the AAPCS,
+	 * which means firmware running at the same exception level as the OS
+	 * (such as UEFI) should never touch it.
+	 */
+	stp	x1, x18, [sp, #16]
+
+	/*
+	 * We are lucky enough that no EFI runtime services take more than
+	 * 5 arguments, so all are passed in registers rather than via the
+	 * stack.
+	 */
+	mov	x8, x0
+	mov	x0, x2
+	mov	x1, x3
+	mov	x2, x4
+	mov	x3, x5
+	mov	x4, x6
+	blr	x8
+
+	ldp	x1, x2, [sp, #16]
+	cmp	x2, x18
+	ldp	x29, x30, [sp], #32
+	b.ne	0f
+	ret
+0:	b	efi_handle_corrupted_x18	// tail call
+ENDPROC(__efi_rt_asm_wrapper)
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index a8bf1c892b90..4f9acb5fbe97 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -126,3 +126,9 @@ bool efi_poweroff_required(void)
 {
 	return efi_enabled(EFI_RUNTIME_SERVICES);
 }
+
+asmlinkage efi_status_t efi_handle_corrupted_x18(efi_status_t s, const char *f)
+{
+	pr_err_ratelimited(FW_BUG "register x18 corrupted by EFI %s\n", f);
+	return s;
+}

[PATCH 06/12] x86: efi: Replace GFP_ATOMIC with GFP_KERNEL in efi_query_variable_store

[Ard Biesheuvel]

From: Jia-Ju Bai <baijiaju1990@gmail.com>

The function kzalloc here is not called in atomic context.
If nonblocking in efi_query_variable_store is true,
namely it is in atomic context, efi_query_variable_store will return before
this kzalloc is called.
Thus GFP_ATOMIC is not necessary, and it can be replaced with GFP_KERNEL.

This is found by a static analysis tool named DCNS written by myself.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/x86/platform/efi/quirks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 5b513ccffde4..1ef11c26f79b 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -177,7 +177,7 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size,
 		 * that by attempting to use more space than is available.
 		 */
 		unsigned long dummy_size = remaining_size + 1024;
-		void *dummy = kzalloc(dummy_size, GFP_ATOMIC);
+		void *dummy = kzalloc(dummy_size, GFP_KERNEL);
 
 		if (!dummy)
 			return EFI_OUT_OF_RESOURCES;
-- 
2.15.1

Re: [PATCH 06/12] x86: efi: Replace GFP_ATOMIC with GFP_KERNEL in efi_query_variable_store

[Ingo Molnar]

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

> From: Jia-Ju Bai <baijiaju1990@gmail.com>
> 
> The function kzalloc here is not called in atomic context.
> If nonblocking in efi_query_variable_store is true,
> namely it is in atomic context, efi_query_variable_store will return before
> this kzalloc is called.
> Thus GFP_ATOMIC is not necessary, and it can be replaced with GFP_KERNEL.
> 
> This is found by a static analysis tool named DCNS written by myself.

Please fix typos/spelling in changelogs when applying patches.

I improved the above to:

  efi_query_variable_store() does an atomic kzalloc() unnecessarily,
  because we can never get this far when called in an atomic context,
  namely when nonblocking == 1.

  Replace it with GFP_KERNEL.

  This was found by the DCNS static analysis tool written by myself.

Thanks,

	Ingo

[tip:efi/core] x86/efi: Replace GFP_ATOMIC with GFP_KERNEL in efi_query_variable_store()

[tip-bot for Jia-Ju Bai]

Commit-ID:  9f66d8d73e654c5f867daa6aa186300ecaf49d3a
Gitweb:     https://git.kernel.org/tip/9f66d8d73e654c5f867daa6aa186300ecaf49d3a
Author:     Jia-Ju Bai <baijiaju1990@gmail.com>
AuthorDate: Thu, 8 Mar 2018 08:00:14 +0000
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 9 Mar 2018 08:58:22 +0100

x86/efi: Replace GFP_ATOMIC with GFP_KERNEL in efi_query_variable_store()

efi_query_variable_store() does an atomic kzalloc() unnecessarily,
because we can never get this far when called in an atomic context,
namely when nonblocking == 1.

Replace it with GFP_KERNEL.

This was found by the DCNS static analysis tool written by myself.

Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-efi@vger.kernel.org
Link: http://lkml.kernel.org/r/20180308080020.22828-7-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/platform/efi/quirks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 5b513ccffde4..1ef11c26f79b 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -177,7 +177,7 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size,
 		 * that by attempting to use more space than is available.
 		 */
 		unsigned long dummy_size = remaining_size + 1024;
-		void *dummy = kzalloc(dummy_size, GFP_ATOMIC);
+		void *dummy = kzalloc(dummy_size, GFP_KERNEL);
 
 		if (!dummy)
 			return EFI_OUT_OF_RESOURCES;

[PATCH 07/12] efi: Use efi_mm in x86 as well as ARM

[Ard Biesheuvel]

From: Sai Praneeth <sai.praneeth.prakhya@intel.com>

Presently, only ARM uses mm_struct to manage efi page tables and efi
runtime region mappings. As this is the preferred approach, let's make
this data structure common across architectures. Specially, for x86,
using this data structure improves code maintainability and readability.

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: "Lee, Chun-Yi" <jlee@suse.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Ricardo Neri <ricardo.neri@intel.com>
Cc: Ravi Shankar <ravi.v.shankar@intel.com>
Tested-by: Bhupesh Sharma <bhsharma@redhat.com>
Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/x86/include/asm/efi.h         | 4 ++++
 arch/x86/platform/efi/efi_64.c     | 3 +++
 drivers/firmware/efi/arm-runtime.c | 9 ---------
 drivers/firmware/efi/efi.c         | 9 +++++++++
 include/linux/efi.h                | 2 ++
 5 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 85f6ccb80b91..00f977ddd718 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -2,10 +2,14 @@
 #ifndef _ASM_X86_EFI_H
 #define _ASM_X86_EFI_H
 
+#include <linux/sched/mm.h>
+#include <linux/sched/task.h>
+
 #include <asm/fpu/api.h>
 #include <asm/pgtable.h>
 #include <asm/processor-flags.h>
 #include <asm/tlb.h>
+#include <asm/mmu_context.h>
 
 /*
  * We map the EFI regions needed for runtime services non-contiguously,
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index c310a8284358..0045efe9947b 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -231,6 +231,9 @@ int __init efi_alloc_page_tables(void)
 		return -ENOMEM;
 	}
 
+	mm_init_cpumask(&efi_mm);
+	init_new_context(NULL, &efi_mm);
+
 	return 0;
 }
 
diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 13561aeb7396..5889cbea60b8 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -31,15 +31,6 @@
 
 extern u64 efi_system_table;
 
-static struct mm_struct efi_mm = {
-	.mm_rb			= RB_ROOT,
-	.mm_users		= ATOMIC_INIT(2),
-	.mm_count		= ATOMIC_INIT(1),
-	.mmap_sem		= __RWSEM_INITIALIZER(efi_mm.mmap_sem),
-	.page_table_lock	= __SPIN_LOCK_UNLOCKED(efi_mm.page_table_lock),
-	.mmlist			= LIST_HEAD_INIT(efi_mm.mmlist),
-};
-
 #ifdef CONFIG_ARM64_PTDUMP_DEBUGFS
 #include <asm/ptdump.h>
 
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index cd42f66a7c85..c0dda400d22a 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -75,6 +75,15 @@ static unsigned long *efi_tables[] = {
 	&efi.mem_attr_table,
 };
 
+struct mm_struct efi_mm = {
+	.mm_rb			= RB_ROOT,
+	.mm_users		= ATOMIC_INIT(2),
+	.mm_count		= ATOMIC_INIT(1),
+	.mmap_sem		= __RWSEM_INITIALIZER(efi_mm.mmap_sem),
+	.page_table_lock	= __SPIN_LOCK_UNLOCKED(efi_mm.page_table_lock),
+	.mmlist			= LIST_HEAD_INIT(efi_mm.mmlist),
+};
+
 static bool disable_runtime;
 static int __init setup_noefi(char *arg)
 {
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f5083aa72eae..f1b7d68ac460 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -966,6 +966,8 @@ extern struct efi {
 	unsigned long flags;
 } efi;
 
+extern struct mm_struct efi_mm;
+
 static inline int
 efi_guidcmp (efi_guid_t left, efi_guid_t right)
 {
-- 
2.15.1

Re: [PATCH 07/12] efi: Use efi_mm in x86 as well as ARM

[Ingo Molnar]

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

> From: Sai Praneeth <sai.praneeth.prakhya@intel.com>
> 
> Presently, only ARM uses mm_struct to manage efi page tables and efi
> runtime region mappings. As this is the preferred approach, let's make
> this data structure common across architectures. Specially, for x86,
> using this data structure improves code maintainability and readability.

> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 85f6ccb80b91..00f977ddd718 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -2,10 +2,14 @@
>  #ifndef _ASM_X86_EFI_H
>  #define _ASM_X86_EFI_H
>  
> +#include <linux/sched/mm.h>
> +#include <linux/sched/task.h>
> +
>  #include <asm/fpu/api.h>
>  #include <asm/pgtable.h>
>  #include <asm/processor-flags.h>
>  #include <asm/tlb.h>
> +#include <asm/mmu_context.h>
>  
>  /*
>   * We map the EFI regions needed for runtime services non-contiguously,

> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index f5083aa72eae..f1b7d68ac460 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -966,6 +966,8 @@ extern struct efi {
>  	unsigned long flags;
>  } efi;
>  
> +extern struct mm_struct efi_mm;
> +
>  static inline int
>  efi_guidcmp (efi_guid_t left, efi_guid_t right)
>  {

Ugh, I can see three problems with this patch:

1)

Why is the low level asm/efi.h header polluted with two of the biggest header 
files in existence, to add a type to _another_ header (efi.h)?

2)

Why is <linux/sched/task.h> included if what is being relied on is mm_struct?

3)

But even <linux/sched/mm.h> looks unnecessary in efi.h, a simple forward 
declaration of mm_struct would do ...

The high level MM and sched headers should be added to the actual .c files that 
make use of them.

Thanks,

	Ingo

RE: [PATCH 07/12] efi: Use efi_mm in x86 as well as ARM

[Prakhya, Sai Praneeth]

> > diff --git a/include/linux/efi.h b/include/linux/efi.h index
> > f5083aa72eae..f1b7d68ac460 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -966,6 +966,8 @@ extern struct efi {
> >  	unsigned long flags;
> >  } efi;
> >
> > +extern struct mm_struct efi_mm;
> > +
> >  static inline int
> >  efi_guidcmp (efi_guid_t left, efi_guid_t right)  {
> 
> Ugh, I can see three problems with this patch:
> 
> 1)
> 
> Why is the low level asm/efi.h header polluted with two of the biggest header
> files in existence, to add a type to _another_ header (efi.h)?
> 
> 2)
> 
> Why is <linux/sched/task.h> included if what is being relied on is mm_struct?
> 
> 3)
> 
> But even <linux/sched/mm.h> looks unnecessary in efi.h, a simple forward
> declaration of mm_struct would do ...
> 
> The high level MM and sched headers should be added to the actual .c files that
> make use of them.

Ok, makes sense.
Sorry! for that. I will fix the issues.

Regards,
Sai

Re: [PATCH 07/12] efi: Use efi_mm in x86 as well as ARM

[Ard Biesheuvel]

On 9 March 2018 at 08:37, Prakhya, Sai Praneeth
<sai.praneeth.prakhya@intel.com> wrote:
>> > diff --git a/include/linux/efi.h b/include/linux/efi.h index
>> > f5083aa72eae..f1b7d68ac460 100644
>> > --- a/include/linux/efi.h
>> > +++ b/include/linux/efi.h
>> > @@ -966,6 +966,8 @@ extern struct efi {
>> >     unsigned long flags;
>> >  } efi;
>> >
>> > +extern struct mm_struct efi_mm;
>> > +
>> >  static inline int
>> >  efi_guidcmp (efi_guid_t left, efi_guid_t right)  {
>>
>> Ugh, I can see three problems with this patch:
>>
>> 1)
>>
>> Why is the low level asm/efi.h header polluted with two of the biggest header
>> files in existence, to add a type to _another_ header (efi.h)?
>>
>> 2)
>>
>> Why is <linux/sched/task.h> included if what is being relied on is mm_struct?
>>
>> 3)
>>
>> But even <linux/sched/mm.h> looks unnecessary in efi.h, a simple forward
>> declaration of mm_struct would do ...
>>
>> The high level MM and sched headers should be added to the actual .c files that
>> make use of them.
>
> Ok, makes sense.
> Sorry! for that. I will fix the issues.
>

I have some other fixups to do, so if this is as easy as it seems
(remove the #includes and add the forward declaration), I can fix it
up and resend it for you.

[PATCH 08/12] x86/efi: Replace efi_pgd with efi_mm.pgd

[Ard Biesheuvel]

From: Sai Praneeth <sai.praneeth.prakhya@intel.com>

Since the previous patch added support for efi_mm, let's handle efi_pgd
through efi_mm and remove global variable efi_pgd.

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: "Lee, Chun-Yi" <jlee@suse.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Ricardo Neri <ricardo.neri@intel.com>
Cc: Ravi Shankar <ravi.v.shankar@intel.com>
Tested-by: Bhupesh Sharma <bhsharma@redhat.com>
Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/x86/platform/efi/efi_64.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 0045efe9947b..8881e601c32d 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -190,8 +190,6 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
 	early_code_mapping_set_exec(0);
 }
 
-static pgd_t *efi_pgd;
-
 /*
  * We need our own copy of the higher levels of the page tables
  * because we want to avoid inserting EFI region mappings (EFI_VA_END
@@ -203,7 +201,7 @@ static pgd_t *efi_pgd;
  */
 int __init efi_alloc_page_tables(void)
 {
-	pgd_t *pgd;
+	pgd_t *pgd, *efi_pgd;
 	p4d_t *p4d;
 	pud_t *pud;
 	gfp_t gfp_mask;
@@ -231,6 +229,7 @@ int __init efi_alloc_page_tables(void)
 		return -ENOMEM;
 	}
 
+	efi_mm.pgd = efi_pgd;
 	mm_init_cpumask(&efi_mm);
 	init_new_context(NULL, &efi_mm);
 
@@ -246,6 +245,7 @@ void efi_sync_low_kernel_mappings(void)
 	pgd_t *pgd_k, *pgd_efi;
 	p4d_t *p4d_k, *p4d_efi;
 	pud_t *pud_k, *pud_efi;
+	pgd_t *efi_pgd = efi_mm.pgd;
 
 	if (efi_enabled(EFI_OLD_MEMMAP))
 		return;
@@ -339,7 +339,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 	unsigned long pfn, text, pf;
 	struct page *page;
 	unsigned npages;
-	pgd_t *pgd;
+	pgd_t *pgd = efi_mm.pgd;
 
 	if (efi_enabled(EFI_OLD_MEMMAP))
 		return 0;
@@ -349,8 +349,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 	 * this value is loaded into cr3 the PGD will be decrypted during
 	 * the pagetable walk.
 	 */
-	efi_scratch.efi_pgt = (pgd_t *)__sme_pa(efi_pgd);
-	pgd = efi_pgd;
+	efi_scratch.efi_pgt = (pgd_t *)__sme_pa(pgd);
 
 	/*
 	 * It can happen that the physical address of new_memmap lands in memory
@@ -420,7 +419,7 @@ static void __init __map_region(efi_memory_desc_t *md, u64 va)
 {
 	unsigned long flags = _PAGE_RW;
 	unsigned long pfn;
-	pgd_t *pgd = efi_pgd;
+	pgd_t *pgd = efi_mm.pgd;
 
 	if (!(md->attribute & EFI_MEMORY_WB))
 		flags |= _PAGE_PCD;
@@ -524,7 +523,7 @@ void __init parse_efi_setup(u64 phys_addr, u32 data_len)
 static int __init efi_update_mappings(efi_memory_desc_t *md, unsigned long pf)
 {
 	unsigned long pfn;
-	pgd_t *pgd = efi_pgd;
+	pgd_t *pgd = efi_mm.pgd;
 	int err1, err2;
 
 	/* Update the 1:1 mapping */
@@ -621,7 +620,7 @@ void __init efi_dump_pagetable(void)
 	if (efi_enabled(EFI_OLD_MEMMAP))
 		ptdump_walk_pgd_level(NULL, swapper_pg_dir);
 	else
-		ptdump_walk_pgd_level(NULL, efi_pgd);
+		ptdump_walk_pgd_level(NULL, efi_mm.pgd);
 #endif
 }
 
-- 
2.15.1

[PATCH 09/12] x86/efi: Use efi_switch_mm() rather than manually twiddling with %cr3

[Ard Biesheuvel]

From: Sai Praneeth <sai.praneeth.prakhya@intel.com>

Use helper function (efi_switch_mm()) to switch to/from efi_mm. We
switch to efi_mm before calling
1. efi_set_virtual_address_map() and
2. Invoking any efi_runtime_service()

Likewise, we need to switch back to previous mm (mm context stolen by
efi_mm) after the above calls return successfully. We can use
efi_switch_mm() helper function only with x86_64 kernel and
"efi=old_map" disabled because, x86_32 and efi=old_map doesn't use
efi_pgd, rather they use swapper_pg_dir.

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: "Lee, Chun-Yi" <jlee@suse.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Ricardo Neri <ricardo.neri@intel.com>
Cc: Ravi Shankar <ravi.v.shankar@intel.com>
Tested-by: Bhupesh Sharma <bhsharma@redhat.com>
Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/x86/include/asm/efi.h           | 25 +++++++++-------------
 arch/x86/platform/efi/efi_64.c       | 40 +++++++++++++++++++-----------------
 arch/x86/platform/efi/efi_thunk_64.S |  2 +-
 3 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 00f977ddd718..cda9940bed7a 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -62,14 +62,13 @@ extern asmlinkage u64 efi_call(void *fp, ...);
 #define efi_call_phys(f, args...)		efi_call((f), args)
 
 /*
- * Scratch space used for switching the pagetable in the EFI stub
+ * struct efi_scratch - Scratch space used while switching to/from efi_mm
+ * @phys_stack: stack used during EFI Mixed Mode
+ * @prev_mm:    store/restore stolen mm_struct while switching to/from efi_mm
  */
 struct efi_scratch {
-	u64	r15;
-	u64	prev_cr3;
-	pgd_t	*efi_pgt;
-	bool	use_pgd;
-	u64	phys_stack;
+	u64			phys_stack;
+	struct mm_struct	*prev_mm;
 } __packed;
 
 #define arch_efi_call_virt_setup()					\
@@ -78,11 +77,8 @@ struct efi_scratch {
 	preempt_disable();						\
 	__kernel_fpu_begin();						\
 									\
-	if (efi_scratch.use_pgd) {					\
-		efi_scratch.prev_cr3 = __read_cr3();			\
-		write_cr3((unsigned long)efi_scratch.efi_pgt);		\
-		__flush_tlb_all();					\
-	}								\
+	if (!efi_enabled(EFI_OLD_MEMMAP))				\
+		efi_switch_mm(&efi_mm);					\
 })
 
 #define arch_efi_call_virt(p, f, args...)				\
@@ -90,10 +86,8 @@ struct efi_scratch {
 
 #define arch_efi_call_virt_teardown()					\
 ({									\
-	if (efi_scratch.use_pgd) {					\
-		write_cr3(efi_scratch.prev_cr3);			\
-		__flush_tlb_all();					\
-	}								\
+	if (!efi_enabled(EFI_OLD_MEMMAP))				\
+		efi_switch_mm(efi_scratch.prev_mm);			\
 									\
 	__kernel_fpu_end();						\
 	preempt_enable();						\
@@ -135,6 +129,7 @@ extern void __init efi_dump_pagetable(void);
 extern void __init efi_apply_memmap_quirks(void);
 extern int __init efi_reuse_config(u64 tables, int nr_tables);
 extern void efi_delete_dummy_variable(void);
+extern void efi_switch_mm(struct mm_struct *mm);
 
 struct efi_setup_data {
 	u64 fw_vendor;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 8881e601c32d..55c3f623ac49 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -81,9 +81,8 @@ pgd_t * __init efi_call_phys_prolog(void)
 	int n_pgds, i, j;
 
 	if (!efi_enabled(EFI_OLD_MEMMAP)) {
-		save_pgd = (pgd_t *)__read_cr3();
-		write_cr3((unsigned long)efi_scratch.efi_pgt);
-		goto out;
+		efi_switch_mm(&efi_mm);
+		return NULL;
 	}
 
 	early_code_mapping_set_exec(1);
@@ -155,8 +154,7 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
 	pud_t *pud;
 
 	if (!efi_enabled(EFI_OLD_MEMMAP)) {
-		write_cr3((unsigned long)save_pgd);
-		__flush_tlb_all();
+		efi_switch_mm(efi_scratch.prev_mm);
 		return;
 	}
 
@@ -344,13 +342,6 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 	if (efi_enabled(EFI_OLD_MEMMAP))
 		return 0;
 
-	/*
-	 * Since the PGD is encrypted, set the encryption mask so that when
-	 * this value is loaded into cr3 the PGD will be decrypted during
-	 * the pagetable walk.
-	 */
-	efi_scratch.efi_pgt = (pgd_t *)__sme_pa(pgd);
-
 	/*
 	 * It can happen that the physical address of new_memmap lands in memory
 	 * which is not mapped in the EFI page table. Therefore we need to go
@@ -364,8 +355,6 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 		return 1;
 	}
 
-	efi_scratch.use_pgd = true;
-
 	/*
 	 * Certain firmware versions are way too sentimential and still believe
 	 * they are exclusive and unquestionable owners of the first physical page,
@@ -624,6 +613,22 @@ void __init efi_dump_pagetable(void)
 #endif
 }
 
+/*
+ * Makes the calling thread switch to/from efi_mm context. Can be used
+ * for SetVirtualAddressMap() i.e. current->active_mm == init_mm as well
+ * as during efi runtime calls i.e current->active_mm == current_mm.
+ * We are not mm_dropping()/mm_grabbing() any mm, because we are not
+ * losing/creating any references.
+ */
+void efi_switch_mm(struct mm_struct *mm)
+{
+	task_lock(current);
+	efi_scratch.prev_mm = current->active_mm;
+	current->active_mm = mm;
+	switch_mm(efi_scratch.prev_mm, mm, NULL);
+	task_unlock(current);
+}
+
 #ifdef CONFIG_EFI_MIXED
 extern efi_status_t efi64_thunk(u32, ...);
 
@@ -677,16 +682,13 @@ efi_status_t efi_thunk_set_virtual_address_map(
 	efi_sync_low_kernel_mappings();
 	local_irq_save(flags);
 
-	efi_scratch.prev_cr3 = __read_cr3();
-	write_cr3((unsigned long)efi_scratch.efi_pgt);
-	__flush_tlb_all();
+	efi_switch_mm(&efi_mm);
 
 	func = (u32)(unsigned long)phys_set_virtual_address_map;
 	status = efi64_thunk(func, memory_map_size, descriptor_size,
 			     descriptor_version, virtual_map);
 
-	write_cr3(efi_scratch.prev_cr3);
-	__flush_tlb_all();
+	efi_switch_mm(efi_scratch.prev_mm);
 	local_irq_restore(flags);
 
 	return status;
diff --git a/arch/x86/platform/efi/efi_thunk_64.S b/arch/x86/platform/efi/efi_thunk_64.S
index 189b218da87c..46c58b08739c 100644
--- a/arch/x86/platform/efi/efi_thunk_64.S
+++ b/arch/x86/platform/efi/efi_thunk_64.S
@@ -33,7 +33,7 @@ ENTRY(efi64_thunk)
 	 * Switch to 1:1 mapped 32-bit stack pointer.
 	 */
 	movq	%rsp, efi_saved_sp(%rip)
-	movq	efi_scratch+25(%rip), %rsp
+	movq	efi_scratch(%rip), %rsp
 
 	/*
 	 * Calculate the physical address of the kernel text.
-- 
2.15.1

[PATCH 10/12] efi: reorder pr_notice() with add_device_randomness() call

[Ard Biesheuvel]

Currently, when we receive a random seed from the EFI stub, we call
add_device_randomness() to incorporate it into the entropy pool, and
issue a pr_notice() saying we are about to do that, e.g.,

  [    0.000000] efi:  RNG=0x87ff92cf18
  [    0.000000] random: fast init done
  [    0.000000] efi: seeding entropy pool

Let's reorder those calls to make the output look less confusing.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/efi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index c0dda400d22a..232f4915223b 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -551,9 +551,9 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
 			seed = early_memremap(efi.rng_seed,
 					      sizeof(*seed) + size);
 			if (seed != NULL) {
+				pr_notice("seeding entropy pool\n");
 				add_device_randomness(seed->bits, seed->size);
 				early_memunmap(seed, sizeof(*seed) + size);
-				pr_notice("seeding entropy pool\n");
 			} else {
 				pr_err("Could not map UEFI random seed!\n");
 			}
-- 
2.15.1

[tip:efi/core] efi: Reorder pr_notice() with add_device_randomness() call

[tip-bot for Ard Biesheuvel]

Commit-ID:  5b4e4c3aa220d07d166d3f21f158bc9c69e3c044
Gitweb:     https://git.kernel.org/tip/5b4e4c3aa220d07d166d3f21f158bc9c69e3c044
Author:     Ard Biesheuvel <ard.biesheuvel@linaro.org>
AuthorDate: Thu, 8 Mar 2018 08:00:18 +0000
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 9 Mar 2018 08:58:23 +0100

efi: Reorder pr_notice() with add_device_randomness() call

Currently, when we receive a random seed from the EFI stub, we call
add_device_randomness() to incorporate it into the entropy pool, and
issue a pr_notice() saying we are about to do that, e.g.,

  [    0.000000] efi:  RNG=0x87ff92cf18
  [    0.000000] random: fast init done
  [    0.000000] efi: seeding entropy pool

Let's reorder those calls to make the output look less confusing:

  [    0.000000] efi: seeding entropy pool
  [    0.000000] efi:  RNG=0x87ff92cf18
  [    0.000000] random: fast init done

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-efi@vger.kernel.org
Link: http://lkml.kernel.org/r/20180308080020.22828-11-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/firmware/efi/efi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index cd42f66a7c85..92b9e79e5da9 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -542,9 +542,9 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
 			seed = early_memremap(efi.rng_seed,
 					      sizeof(*seed) + size);
 			if (seed != NULL) {
+				pr_notice("seeding entropy pool\n");
 				add_device_randomness(seed->bits, seed->size);
 				early_memunmap(seed, sizeof(*seed) + size);
-				pr_notice("seeding entropy pool\n");
 			} else {
 				pr_err("Could not map UEFI random seed!\n");
 			}

[PATCH 11/12] efi/apple-properties: Use memremap() instead of ioremap()

[Ard Biesheuvel]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The memory we are accessing through virtual address has no IO side
effects. Moreover, for IO memory we have to use special accessors,
which we don't use.

Due to above, convert the driver to use memremap() instead of ioremap().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/apple-properties.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/apple-properties.c b/drivers/firmware/efi/apple-properties.c
index b9602e0d7b50..adaa9a3714b9 100644
--- a/drivers/firmware/efi/apple-properties.c
+++ b/drivers/firmware/efi/apple-properties.c
@@ -19,6 +19,7 @@
 
 #include <linux/bootmem.h>
 #include <linux/efi.h>
+#include <linux/io.h>
 #include <linux/platform_data/x86/apple.h>
 #include <linux/property.h>
 #include <linux/slab.h>
@@ -189,7 +190,7 @@ static int __init map_properties(void)
 
 	pa_data = boot_params.hdr.setup_data;
 	while (pa_data) {
-		data = ioremap(pa_data, sizeof(*data));
+		data = memremap(pa_data, sizeof(*data), MEMREMAP_WB);
 		if (!data) {
 			pr_err("cannot map setup_data header\n");
 			return -ENOMEM;
@@ -197,14 +198,14 @@ static int __init map_properties(void)
 
 		if (data->type != SETUP_APPLE_PROPERTIES) {
 			pa_data = data->next;
-			iounmap(data);
+			memunmap(data);
 			continue;
 		}
 
 		data_len = data->len;
-		iounmap(data);
+		memunmap(data);
 
-		data = ioremap(pa_data, sizeof(*data) + data_len);
+		data = memremap(pa_data, sizeof(*data) + data_len, MEMREMAP_WB);
 		if (!data) {
 			pr_err("cannot map setup_data payload\n");
 			return -ENOMEM;
@@ -229,7 +230,7 @@ static int __init map_properties(void)
 		 * to avoid breaking the chain of ->next pointers.
 		 */
 		data->len = 0;
-		iounmap(data);
+		memunmap(data);
 		free_bootmem_late(pa_data + sizeof(*data), data_len);
 
 		return ret;
-- 
2.15.1

[tip:efi/core] efi/apple-properties: Use memremap() instead of ioremap()

[tip-bot for Andy Shevchenko]

Commit-ID:  44612d7e0c379001460b37a29721128715bdcb02
Gitweb:     https://git.kernel.org/tip/44612d7e0c379001460b37a29721128715bdcb02
Author:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
AuthorDate: Thu, 8 Mar 2018 08:00:19 +0000
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 9 Mar 2018 08:58:23 +0100

efi/apple-properties: Use memremap() instead of ioremap()

The memory we are accessing through virtual address has no IO side
effects. Moreover, for IO memory we have to use special accessors,
which we don't use.

Due to above, convert the driver to use memremap() instead of ioremap().

Tested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-efi@vger.kernel.org
Link: http://lkml.kernel.org/r/20180308080020.22828-12-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/firmware/efi/apple-properties.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/apple-properties.c b/drivers/firmware/efi/apple-properties.c
index b9602e0d7b50..adaa9a3714b9 100644
--- a/drivers/firmware/efi/apple-properties.c
+++ b/drivers/firmware/efi/apple-properties.c
@@ -19,6 +19,7 @@
 
 #include <linux/bootmem.h>
 #include <linux/efi.h>
+#include <linux/io.h>
 #include <linux/platform_data/x86/apple.h>
 #include <linux/property.h>
 #include <linux/slab.h>
@@ -189,7 +190,7 @@ static int __init map_properties(void)
 
 	pa_data = boot_params.hdr.setup_data;
 	while (pa_data) {
-		data = ioremap(pa_data, sizeof(*data));
+		data = memremap(pa_data, sizeof(*data), MEMREMAP_WB);
 		if (!data) {
 			pr_err("cannot map setup_data header\n");
 			return -ENOMEM;
@@ -197,14 +198,14 @@ static int __init map_properties(void)
 
 		if (data->type != SETUP_APPLE_PROPERTIES) {
 			pa_data = data->next;
-			iounmap(data);
+			memunmap(data);
 			continue;
 		}
 
 		data_len = data->len;
-		iounmap(data);
+		memunmap(data);
 
-		data = ioremap(pa_data, sizeof(*data) + data_len);
+		data = memremap(pa_data, sizeof(*data) + data_len, MEMREMAP_WB);
 		if (!data) {
 			pr_err("cannot map setup_data payload\n");
 			return -ENOMEM;
@@ -229,7 +230,7 @@ static int __init map_properties(void)
 		 * to avoid breaking the chain of ->next pointers.
 		 */
 		data->len = 0;
-		iounmap(data);
+		memunmap(data);
 		free_bootmem_late(pa_data + sizeof(*data), data_len);
 
 		return ret;

[PATCH 12/12] efi: make const array 'apple' static

[Ard Biesheuvel]

From: Colin Ian King <colin.king@canonical.com>

Don't populate the const read-only array 'buf' on the stack but instead
make it static. Makes the object code smaller by 64 bytes:

Before:
   text	   data	    bss	    dec	    hex	filename
   9264	      1	     16	   9281	   2441	arch/x86/boot/compressed/eboot.o

After:
   text	   data	    bss	    dec	    hex	filename
   9200	      1	     16	   9217	   2401	arch/x86/boot/compressed/eboot.o

(gcc version 7.2.0 x86_64)

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/x86/boot/compressed/eboot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 886a9115af62..f2251c1c9853 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
 
 static void setup_quirks(struct boot_params *boot_params)
 {
-	efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
+	static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
 	efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
 		efi_table_attr(efi_system_table, fw_vendor, sys_table);
 
-- 
2.15.1

Re: [PATCH 12/12] efi: make const array 'apple' static

[Joe Perches]

On Thu, 2018-03-08 at 08:00 +0000, Ard Biesheuvel wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Don't populate the const read-only array 'buf' on the stack but instead
> make it static. Makes the object code smaller by 64 bytes:
> 
> Before:
>    text	   data	    bss	    dec	    hex	filename
>    9264	      1	     16	   9281	   2441	arch/x86/boot/compressed/eboot.o
> 
> After:
>    text	   data	    bss	    dec	    hex	filename
>    9200	      1	     16	   9217	   2401	arch/x86/boot/compressed/eboot.o
> 
> (gcc version 7.2.0 x86_64)
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/x86/boot/compressed/eboot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 886a9115af62..f2251c1c9853 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
>  
>  static void setup_quirks(struct boot_params *boot_params)
>  {
> -	efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> +	static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };

Perhaps 

static const efi_char16_t apple[] ...

is better.

>  	efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
>  		efi_table_attr(efi_system_table, fw_vendor, sys_table);
>  

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ard Biesheuvel]

On 8 March 2018 at 11:05, Joe Perches <joe@perches.com> wrote:
> On Thu, 2018-03-08 at 08:00 +0000, Ard Biesheuvel wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Don't populate the const read-only array 'buf' on the stack but instead
>> make it static. Makes the object code smaller by 64 bytes:
>>
>> Before:
>>    text          data     bss     dec     hex filename
>>    9264             1      16    9281    2441 arch/x86/boot/compressed/eboot.o
>>
>> After:
>>    text          data     bss     dec     hex filename
>>    9200             1      16    9217    2401 arch/x86/boot/compressed/eboot.o
>>
>> (gcc version 7.2.0 x86_64)
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/x86/boot/compressed/eboot.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
>> index 886a9115af62..f2251c1c9853 100644
>> --- a/arch/x86/boot/compressed/eboot.c
>> +++ b/arch/x86/boot/compressed/eboot.c
>> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
>>
>>  static void setup_quirks(struct boot_params *boot_params)
>>  {
>> -     efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>> +     static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>
> Perhaps
>
> static const efi_char16_t apple[] ...
>
> is better.
>

Why would that be any better? I have always found the 'const'
placement after the type to be much clearer.

const void *
void const *
void * const

I.e., #2 and #3 are equivalent, and so 'const' associates to the left
not to the right, unless it is at the beginning.

Personally, I don't mind either way, but saying it is 'better' is a stretch imo

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ard Biesheuvel]

On 9 March 2018 at 07:43, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 8 March 2018 at 11:05, Joe Perches <joe@perches.com> wrote:
>> On Thu, 2018-03-08 at 08:00 +0000, Ard Biesheuvel wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> Don't populate the const read-only array 'buf' on the stack but instead
>>> make it static. Makes the object code smaller by 64 bytes:
>>>
>>> Before:
>>>    text          data     bss     dec     hex filename
>>>    9264             1      16    9281    2441 arch/x86/boot/compressed/eboot.o
>>>
>>> After:
>>>    text          data     bss     dec     hex filename
>>>    9200             1      16    9217    2401 arch/x86/boot/compressed/eboot.o
>>>
>>> (gcc version 7.2.0 x86_64)
>>>
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  arch/x86/boot/compressed/eboot.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
>>> index 886a9115af62..f2251c1c9853 100644
>>> --- a/arch/x86/boot/compressed/eboot.c
>>> +++ b/arch/x86/boot/compressed/eboot.c
>>> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
>>>
>>>  static void setup_quirks(struct boot_params *boot_params)
>>>  {
>>> -     efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>>> +     static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>>
>> Perhaps
>>
>> static const efi_char16_t apple[] ...
>>
>> is better.
>>
>
> Why would that be any better? I have always found the 'const'
> placement after the type to be much clearer.
>
> const void *
> void const *
> void * const
>
> I.e., #2 and #3 are equivalent,

That would be #1 and #2, of course

> and so 'const' associates to the left
> not to the right, unless it is at the beginning.
>
> Personally, I don't mind either way, but saying it is 'better' is a stretch imo

Re: [PATCH 12/12] efi: make const array 'apple' static

[Joe Perches]

On Fri, 2018-03-09 at 07:44 +0000, Ard Biesheuvel wrote:
> On 9 March 2018 at 07:43, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > On 8 March 2018 at 11:05, Joe Perches <joe@perches.com> wrote:
> > > On Thu, 2018-03-08 at 08:00 +0000, Ard Biesheuvel wrote:
> > > > From: Colin Ian King <colin.king@canonical.com>
> > > > 
> > > > Don't populate the const read-only array 'buf' on the stack but instead
> > > > make it static. Makes the object code smaller by 64 bytes:
> > > > 
> > > > Before:
> > > >    text          data     bss     dec     hex filename
> > > >    9264             1      16    9281    2441 arch/x86/boot/compressed/eboot.o
> > > > 
> > > > After:
> > > >    text          data     bss     dec     hex filename
> > > >    9200             1      16    9217    2401 arch/x86/boot/compressed/eboot.o
> > > > 
> > > > (gcc version 7.2.0 x86_64)
> > > > 
> > > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > ---
> > > >  arch/x86/boot/compressed/eboot.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > > > index 886a9115af62..f2251c1c9853 100644
> > > > --- a/arch/x86/boot/compressed/eboot.c
> > > > +++ b/arch/x86/boot/compressed/eboot.c
> > > > @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
> > > > 
> > > >  static void setup_quirks(struct boot_params *boot_params)
> > > >  {
> > > > -     efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> > > > +     static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> > > 
> > > Perhaps
> > > 
> > > static const efi_char16_t apple[] ...
> > > 
> > > is better.
> > > 
> > 
> > Why would that be any better?

It'd be more like the the style used
in the rest of the kernel.

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ingo Molnar]

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

> From: Colin Ian King <colin.king@canonical.com>
> 
> Don't populate the const read-only array 'buf' on the stack but instead
> make it static. Makes the object code smaller by 64 bytes:
> 
> Before:
>    text	   data	    bss	    dec	    hex	filename
>    9264	      1	     16	   9281	   2441	arch/x86/boot/compressed/eboot.o
> 
> After:
>    text	   data	    bss	    dec	    hex	filename
>    9200	      1	     16	   9217	   2401	arch/x86/boot/compressed/eboot.o
> 
> (gcc version 7.2.0 x86_64)
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/x86/boot/compressed/eboot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 886a9115af62..f2251c1c9853 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
>  
>  static void setup_quirks(struct boot_params *boot_params)
>  {
> -	efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> +	static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>  	efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
>  		efi_table_attr(efi_system_table, fw_vendor, sys_table);

As a general policy, please don't put 'static' variables into the local scope,
use file scope instead - right before setup_quirks() would be fine.

This makes it abundantly clear that it's not on the stack.

Also, would it make sense to rename it to something more descriptive like 
"apple_unicode_str[]" or so?

Plus an unicode string literal initializer would be pretty descriptive as well, 
instead of the weird looking character array, i.e. something like:

  static efi_char16_t const apple_unicode_str[] = u"Apple";

... or so?

Thanks,

	Ingo

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ard Biesheuvel]

On 9 March 2018 at 07:47, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Don't populate the const read-only array 'buf' on the stack but instead
>> make it static. Makes the object code smaller by 64 bytes:
>>
>> Before:
>>    text          data     bss     dec     hex filename
>>    9264             1      16    9281    2441 arch/x86/boot/compressed/eboot.o
>>
>> After:
>>    text          data     bss     dec     hex filename
>>    9200             1      16    9217    2401 arch/x86/boot/compressed/eboot.o
>>
>> (gcc version 7.2.0 x86_64)
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/x86/boot/compressed/eboot.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
>> index 886a9115af62..f2251c1c9853 100644
>> --- a/arch/x86/boot/compressed/eboot.c
>> +++ b/arch/x86/boot/compressed/eboot.c
>> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
>>
>>  static void setup_quirks(struct boot_params *boot_params)
>>  {
>> -     efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>> +     static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>>       efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
>>               efi_table_attr(efi_system_table, fw_vendor, sys_table);
>
> As a general policy, please don't put 'static' variables into the local scope,
> use file scope instead - right before setup_quirks() would be fine.
>
> This makes it abundantly clear that it's not on the stack.
>

Fair enough. I didn't know there was such a policy, but since these
have local scope by definition, it doesn't pollute the global
namespace so it's fine

> Also, would it make sense to rename it to something more descriptive like
> "apple_unicode_str[]" or so?
>
> Plus an unicode string literal initializer would be pretty descriptive as well,
> instead of the weird looking character array, i.e. something like:
>
>   static efi_char16_t const apple_unicode_str[] = u"Apple";
>
> ... or so?
>

is u"xxx" the same as L"xxx"?

In any case, this is for historical reasons: at some point (and I
don't remember the exact details) we had a conflict at link time with
objects using 4 byte wchar_t, so we started using this notation to be
independent of the size of wchar_t. That issue no longer exists so we
should be able to get rid of this.

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ingo Molnar]

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

> > Also, would it make sense to rename it to something more descriptive like
> > "apple_unicode_str[]" or so?
> >
> > Plus an unicode string literal initializer would be pretty descriptive as well,
> > instead of the weird looking character array, i.e. something like:
> >
> >   static efi_char16_t const apple_unicode_str[] = u"Apple";
> >
> > ... or so?
> >
> 
> is u"xxx" the same as L"xxx"?

So "L" literals map to wchar_t, which wide character type is implementation 
specific IIRC, could be 16-bit or 32-bit wide.

u"" literals OTOH are specified by the C11 spec to be char16_t, i.e. 16-bit wide 
characters - which I assume is the EFI type as well?

> In any case, this is for historical reasons: at some point (and I
> don't remember the exact details) we had a conflict at link time with
> objects using 4 byte wchar_t, so we started using this notation to be
> independent of the size of wchar_t. That issue no longer exists so we
> should be able to get rid of this.

Yes, my guess is that those problems were due to L"xyz" mapping to wchar_t and 
having a different type in the kernel build and the host build side - but u"xyz" 
should solve that.

Thanks,

	Ingo

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ard Biesheuvel]

On 9 March 2018 at 08:04, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> > Also, would it make sense to rename it to something more descriptive like
>> > "apple_unicode_str[]" or so?
>> >
>> > Plus an unicode string literal initializer would be pretty descriptive as well,
>> > instead of the weird looking character array, i.e. something like:
>> >
>> >   static efi_char16_t const apple_unicode_str[] = u"Apple";
>> >
>> > ... or so?
>> >
>>
>> is u"xxx" the same as L"xxx"?
>
> So "L" literals map to wchar_t, which wide character type is implementation
> specific IIRC, could be 16-bit or 32-bit wide.
>
> u"" literals OTOH are specified by the C11 spec to be char16_t, i.e. 16-bit wide
> characters - which I assume is the EFI type as well?
>
>> In any case, this is for historical reasons: at some point (and I
>> don't remember the exact details) we had a conflict at link time with
>> objects using 4 byte wchar_t, so we started using this notation to be
>> independent of the size of wchar_t. That issue no longer exists so we
>> should be able to get rid of this.
>
> Yes, my guess is that those problems were due to L"xyz" mapping to wchar_t and
> having a different type in the kernel build and the host build side - but u"xyz"
> should solve that.
>

Excellent!

Do you mind taking this patch as is? I will follow up with a patch
that updates all occurrences of this pattern (we have a few of them),
i.e., use u"" notation and move them to file scope.

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ard Biesheuvel]

On 9 March 2018 at 08:07, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 9 March 2018 at 08:04, Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>>> > Also, would it make sense to rename it to something more descriptive like
>>> > "apple_unicode_str[]" or so?
>>> >
>>> > Plus an unicode string literal initializer would be pretty descriptive as well,
>>> > instead of the weird looking character array, i.e. something like:
>>> >
>>> >   static efi_char16_t const apple_unicode_str[] = u"Apple";
>>> >
>>> > ... or so?
>>> >
>>>
>>> is u"xxx" the same as L"xxx"?
>>
>> So "L" literals map to wchar_t, which wide character type is implementation
>> specific IIRC, could be 16-bit or 32-bit wide.
>>
>> u"" literals OTOH are specified by the C11 spec to be char16_t, i.e. 16-bit wide
>> characters - which I assume is the EFI type as well?
>>
>>> In any case, this is for historical reasons: at some point (and I
>>> don't remember the exact details) we had a conflict at link time with
>>> objects using 4 byte wchar_t, so we started using this notation to be
>>> independent of the size of wchar_t. That issue no longer exists so we
>>> should be able to get rid of this.
>>
>> Yes, my guess is that those problems were due to L"xyz" mapping to wchar_t and
>> having a different type in the kernel build and the host build side - but u"xyz"
>> should solve that.
>>
>
> Excellent!
>
> Do you mind taking this patch as is? I will follow up with a patch
> that updates all occurrences of this pattern (we have a few of them),
> i.e., use u"" notation and move them to file scope.

OK, I misremembered: the other occurrences have already been moved to
file scope a while ago.

I will follow up with a patch that switches to u"" notation, please
let me know if I should respin this or not

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ingo Molnar]

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

> On 9 March 2018 at 08:04, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> >> > Also, would it make sense to rename it to something more descriptive like
> >> > "apple_unicode_str[]" or so?
> >> >
> >> > Plus an unicode string literal initializer would be pretty descriptive as well,
> >> > instead of the weird looking character array, i.e. something like:
> >> >
> >> >   static efi_char16_t const apple_unicode_str[] = u"Apple";
> >> >
> >> > ... or so?
> >> >
> >>
> >> is u"xxx" the same as L"xxx"?
> >
> > So "L" literals map to wchar_t, which wide character type is implementation
> > specific IIRC, could be 16-bit or 32-bit wide.
> >
> > u"" literals OTOH are specified by the C11 spec to be char16_t, i.e. 16-bit wide
> > characters - which I assume is the EFI type as well?
> >
> >> In any case, this is for historical reasons: at some point (and I
> >> don't remember the exact details) we had a conflict at link time with
> >> objects using 4 byte wchar_t, so we started using this notation to be
> >> independent of the size of wchar_t. That issue no longer exists so we
> >> should be able to get rid of this.
> >
> > Yes, my guess is that those problems were due to L"xyz" mapping to wchar_t and
> > having a different type in the kernel build and the host build side - but u"xyz"
> > should solve that.
> >
> 
> Excellent!

Please double check the generated code though, all of this is from memory.

> Do you mind taking this patch as is? I will follow up with a patch
> that updates all occurrences of this pattern (we have a few of them),
> i.e., use u"" notation and move them to file scope.

Sure, done!

Thanks,

	Ingo

Re: [PATCH 12/12] efi: make const array 'apple' static

[Lukas Wunner]

On Fri, Mar 09, 2018 at 08:47:19AM +0100, Ingo Molnar wrote:
> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > Don't populate the const read-only array 'buf' on the stack but instead
> > make it static. Makes the object code smaller by 64 bytes:
> > 
> > Before:
> >    text	   data	    bss	    dec	    hex	filename
> >    9264	      1	     16	   9281	   2441	arch/x86/boot/compressed/eboot.o
> > 
> > After:
> >    text	   data	    bss	    dec	    hex	filename
> >    9200	      1	     16	   9217	   2401	arch/x86/boot/compressed/eboot.o
> > 
> > (gcc version 7.2.0 x86_64)
> > 
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  arch/x86/boot/compressed/eboot.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index 886a9115af62..f2251c1c9853 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
> >  
> >  static void setup_quirks(struct boot_params *boot_params)
> >  {
> > -	efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> > +	static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> >  	efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
> >  		efi_table_attr(efi_system_table, fw_vendor, sys_table);
> 
> As a general policy, please don't put 'static' variables into the local
> scope, use file scope instead - right before setup_quirks() would be fine.

Well, I believe the end result is the same and the closer the declaration
is to where it's used, the easier the code is to read and understand.

I object to patches like this because they paper over a missing
compiler optimization:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725

I have told Colin before that it would be more useful to look into
fixing the underlying compiler issue rather than polluting the kernel
with "static" keywords, but he keeps sending these patches so I've
given up responding:
https://lkml.org/lkml/2017/8/25/636


> Plus an unicode string literal initializer would be pretty descriptive
> as well,  instead of the weird looking character array, i.e. something
> like:
> 
>   static efi_char16_t const apple_unicode_str[] = u"Apple";
> 
> ... or so?

Last time I checked this didn't work, I believe it's because it's C11
and the kernel is compiled with -std=gnu89.  And I'm also not sure if
the oldest gcc version that we support understands u"".

Thanks,

Lukas

Re: [PATCH 12/12] efi: make const array 'apple' static

[Ard Biesheuvel]

Hi Lukas,

On 9 March 2018 at 08:29, Lukas Wunner <lukas@wunner.de> wrote:
> On Fri, Mar 09, 2018 at 08:47:19AM +0100, Ingo Molnar wrote:
>> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> > From: Colin Ian King <colin.king@canonical.com>
>> >
>> > Don't populate the const read-only array 'buf' on the stack but instead
>> > make it static. Makes the object code smaller by 64 bytes:
>> >
>> > Before:
>> >    text        data     bss     dec     hex filename
>> >    9264           1      16    9281    2441 arch/x86/boot/compressed/eboot.o
>> >
>> > After:
>> >    text        data     bss     dec     hex filename
>> >    9200           1      16    9217    2401 arch/x86/boot/compressed/eboot.o
>> >
>> > (gcc version 7.2.0 x86_64)
>> >
>> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > ---
>> >  arch/x86/boot/compressed/eboot.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
>> > index 886a9115af62..f2251c1c9853 100644
>> > --- a/arch/x86/boot/compressed/eboot.c
>> > +++ b/arch/x86/boot/compressed/eboot.c
>> > @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
>> >
>> >  static void setup_quirks(struct boot_params *boot_params)
>> >  {
>> > -   efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>> > +   static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>> >     efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
>> >             efi_table_attr(efi_system_table, fw_vendor, sys_table);
>>
>> As a general policy, please don't put 'static' variables into the local
>> scope, use file scope instead - right before setup_quirks() would be fine.
>
> Well, I believe the end result is the same and the closer the declaration
> is to where it's used, the easier the code is to read and understand.
>
> I object to patches like this because they paper over a missing
> compiler optimization:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725
>
> I have told Colin before that it would be more useful to look into
> fixing the underlying compiler issue rather than polluting the kernel
> with "static" keywords, but he keeps sending these patches so I've
> given up responding:
> https://lkml.org/lkml/2017/8/25/636
>
>
>> Plus an unicode string literal initializer would be pretty descriptive
>> as well,  instead of the weird looking character array, i.e. something
>> like:
>>
>>   static efi_char16_t const apple_unicode_str[] = u"Apple";
>>
>> ... or so?
>
> Last time I checked this didn't work, I believe it's because it's C11
> and the kernel is compiled with -std=gnu89.  And I'm also not sure if
> the oldest gcc version that we support understands u"".
>

Indeed

arch/x86/platform/efi/quirks.c:78:46: error: 'u' undeclared here (not
in a function); did you mean 'up'?

 static const efi_char16_t efi_dummy_name[] = u"DUMMY";

[tip:efi/core] efi: Make const array 'apple' static

[tip-bot for Colin Ian King]

Commit-ID:  f779ca740f25c8a6a72d951334f9efc3158a318b
Gitweb:     https://git.kernel.org/tip/f779ca740f25c8a6a72d951334f9efc3158a318b
Author:     Colin Ian King <colin.king@canonical.com>
AuthorDate: Thu, 8 Mar 2018 08:00:20 +0000
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 9 Mar 2018 09:30:35 +0100

efi: Make const array 'apple' static

Don't populate the const read-only array 'buf' on the stack but instead
make it static. Makes the object code smaller by 64 bytes:

Before:
   text	   data	    bss	    dec	    hex	filename
   9264	      1	     16	   9281	   2441	arch/x86/boot/compressed/eboot.o

After:
   text	   data	    bss	    dec	    hex	filename
   9200	      1	     16	   9217	   2401	arch/x86/boot/compressed/eboot.o

(GCC version 7.2.0 x86_64)

Signed-off-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-efi@vger.kernel.org
Link: http://lkml.kernel.org/r/20180308080020.22828-13-ard.biesheuvel@linaro.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/boot/compressed/eboot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 886a9115af62..f2251c1c9853 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct boot_params *boot_params)
 
 static void setup_quirks(struct boot_params *boot_params)
 {
-	efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
+	static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
 	efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
 		efi_table_attr(efi_system_table, fw_vendor, sys_table);