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
From: Luis de Bethencourt <[email protected]>
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 <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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
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 <[email protected]>
---
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
From: Colin Ian King <[email protected]>
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 <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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
From: Andy Shevchenko <[email protected]>
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 <[email protected]>
Tested-by: Lukas Wunner <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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
From: Sai Praneeth <[email protected]>
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 <[email protected]>
Cc: "Lee, Chun-Yi" <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Michael S. Tsirkin <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Ricardo Neri <[email protected]>
Cc: Ravi Shankar <[email protected]>
Tested-by: Bhupesh Sharma <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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
From: Sai Praneeth <[email protected]>
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 <[email protected]>
Cc: "Lee, Chun-Yi" <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Michael S. Tsirkin <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Ricardo Neri <[email protected]>
Cc: Ravi Shankar <[email protected]>
Tested-by: Bhupesh Sharma <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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
From: Jia-Ju Bai <[email protected]>
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 <[email protected]>
Reviewed-by: Ingo Molnar <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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
From: Sai Praneeth <[email protected]>
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 <[email protected]>
Cc: "Lee, Chun-Yi" <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Michael S. Tsirkin <[email protected]>
Cc: Ricardo Neri <[email protected]>
Cc: Ravi Shankar <[email protected]>
Tested-by: Bhupesh Sharma <[email protected]>
Reviewed-by: Matt Fleming <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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 <[email protected]>
+ *
+ * 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
From: Mark Rutland <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Rutland <[email protected]>
Acked-by: Will Deacon <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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
From: Andy Shevchenko <[email protected]>
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 <[email protected]>
Reviewed-and-tested-by: Lukas Wunner <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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
On Thu, 2018-03-08 at 08:00 +0000, Ard Biesheuvel wrote:
> From: Colin Ian King <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> 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);
>
* Ard Biesheuvel <[email protected]> wrote:
> From: Sai Praneeth <[email protected]>
>
> 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
On 8 March 2018 at 11:05, Joe Perches <[email protected]> wrote:
> On Thu, 2018-03-08 at 08:00 +0000, Ard Biesheuvel wrote:
>> From: Colin Ian King <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Ard Biesheuvel <[email protected]>
>> ---
>> 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
On 9 March 2018 at 07:43, Ard Biesheuvel <[email protected]> wrote:
> On 8 March 2018 at 11:05, Joe Perches <[email protected]> wrote:
>> On Thu, 2018-03-08 at 08:00 +0000, Ard Biesheuvel wrote:
>>> From: Colin Ian King <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Ard Biesheuvel <[email protected]>
>>> ---
>>> 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
* Ard Biesheuvel <[email protected]> wrote:
> From: Colin Ian King <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> 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
On 9 March 2018 at 07:47, Ingo Molnar <[email protected]> wrote:
>
> * Ard Biesheuvel <[email protected]> wrote:
>
>> From: Colin Ian King <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Ard Biesheuvel <[email protected]>
>> ---
>> 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.
* Ard Biesheuvel <[email protected]> wrote:
> From: Jia-Ju Bai <[email protected]>
>
> 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
* Ard Biesheuvel <[email protected]> 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
On 9 March 2018 at 08:04, Ingo Molnar <[email protected]> wrote:
>
> * Ard Biesheuvel <[email protected]> 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.
On 9 March 2018 at 08:07, Ard Biesheuvel <[email protected]> wrote:
> On 9 March 2018 at 08:04, Ingo Molnar <[email protected]> wrote:
>>
>> * Ard Biesheuvel <[email protected]> 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
On Fri, Mar 09, 2018 at 08:47:19AM +0100, Ingo Molnar wrote:
> * Ard Biesheuvel <[email protected]> wrote:
> > From: Colin Ian King <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Ard Biesheuvel <[email protected]>
> > ---
> > 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
* Ard Biesheuvel <[email protected]> wrote:
> On 9 March 2018 at 08:04, Ingo Molnar <[email protected]> wrote:
> >
> > * Ard Biesheuvel <[email protected]> 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
Hi Lukas,
On 9 March 2018 at 08:29, Lukas Wunner <[email protected]> wrote:
> On Fri, Mar 09, 2018 at 08:47:19AM +0100, Ingo Molnar wrote:
>> * Ard Biesheuvel <[email protected]> wrote:
>> > From: Colin Ian King <[email protected]>
>> >
>> > 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 <[email protected]>
>> > Signed-off-by: Ard Biesheuvel <[email protected]>
>> > ---
>> > 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";
> > 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
Commit-ID: 6b31a2fa1e8f7bc6c2a474b4a12dad7a145cf83d
Gitweb: https://git.kernel.org/tip/6b31a2fa1e8f7bc6c2a474b4a12dad7a145cf83d
Author: Mark Rutland <[email protected]>
AuthorDate: Thu, 8 Mar 2018 08:00:09 +0000
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Rutland <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Acked-by: Will Deacon <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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);
Commit-ID: 7e611e7dbb235938fca1dd359bad5e5f86ceabcb
Gitweb: https://git.kernel.org/tip/7e611e7dbb235938fca1dd359bad5e5f86ceabcb
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Thu, 8 Mar 2018 08:00:13 +0000
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Acked-by: Will Deacon <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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 <[email protected]>
+ *
+ * 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;
+}
Commit-ID: 1832e64162ffbbbdf7230401298550f2b624351b
Gitweb: https://git.kernel.org/tip/1832e64162ffbbbdf7230401298550f2b624351b
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Thu, 8 Mar 2018 08:00:11 +0000
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Acked-by: Leif Lindholm <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
Commit-ID: 6e98503dba64e721ba839e75dca036266ec0140f
Gitweb: https://git.kernel.org/tip/6e98503dba64e721ba839e75dca036266ec0140f
Author: Andy Shevchenko <[email protected]>
AuthorDate: Thu, 8 Mar 2018 08:00:10 +0000
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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);
Commit-ID: 9f66d8d73e654c5f867daa6aa186300ecaf49d3a
Gitweb: https://git.kernel.org/tip/9f66d8d73e654c5f867daa6aa186300ecaf49d3a
Author: Jia-Ju Bai <[email protected]>
AuthorDate: Thu, 8 Mar 2018 08:00:14 +0000
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
Commit-ID: 5b4e4c3aa220d07d166d3f21f158bc9c69e3c044
Gitweb: https://git.kernel.org/tip/5b4e4c3aa220d07d166d3f21f158bc9c69e3c044
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Thu, 8 Mar 2018 08:00:18 +0000
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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");
}
Commit-ID: 44612d7e0c379001460b37a29721128715bdcb02
Gitweb: https://git.kernel.org/tip/44612d7e0c379001460b37a29721128715bdcb02
Author: Andy Shevchenko <[email protected]>
AuthorDate: Thu, 8 Mar 2018 08:00:19 +0000
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
Commit-ID: f779ca740f25c8a6a72d951334f9efc3158a318b
Gitweb: https://git.kernel.org/tip/f779ca740f25c8a6a72d951334f9efc3158a318b
Author: Colin Ian King <[email protected]>
AuthorDate: Thu, 8 Mar 2018 08:00:20 +0000
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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);
On Fri, 2018-03-09 at 07:44 +0000, Ard Biesheuvel wrote:
> On 9 March 2018 at 07:43, Ard Biesheuvel <[email protected]> wrote:
> > On 8 March 2018 at 11:05, Joe Perches <[email protected]> wrote:
> > > On Thu, 2018-03-08 at 08:00 +0000, Ard Biesheuvel wrote:
> > > > From: Colin Ian King <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > Signed-off-by: Ard Biesheuvel <[email protected]>
> > > > ---
> > > > 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.
On 9 March 2018 at 08:37, Prakhya, Sai Praneeth
<[email protected]> 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.