The following changes since commit 976b489120cdab2b1b3a41ffa14661db43d58190:
efi: Prevent GICv3 WARN() by mapping the memreserve table before first use (2018-11-27 13:50:20 +0100)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-next
for you to fetch changes up to 1d29afdbf7ae878a23627ebee81efcd213f11749:
efi/x86: earlyprintk - Fix infinite loop on some screen widths (2018-11-28 17:58:42 +0100)
----------------------------------------------------------------
EFI updates for v4.21:
- Allocate the E820 buffer before doing the GetMemoryMap/ExitBootServices
dance so we don't run out of space (Eric)
- Clear EFI boot services mappings when freeing the memory (Sai)
- Harden efivars against callers that invoke it on non-EFI boots (Arend)
- Reduce the number of memblock reservations resulting from extensive
use of the new efi_mem_reserve_persistent() API (Ard)
- Other assorted fixes and cleanups.
----------------------------------------------------------------
Ard Biesheuvel (2):
efi: permit multiple entries in persistent memreserve data structure
efi: reduce the amount of memblock reservations for persistent allocations
Arend van Spriel (1):
firmware: efi: add NULL pointer checks in efivars api functions
Eric Snowberg (1):
x86/efi: Allocate e820 buffer before calling efi_exit_boot_service
Julien Thierry (2):
efi/fdt: Indentation fix
efi/fdt: Simplify get_fdt flow
Nathan Chancellor (1):
efi/libstub: Disable some warnings for x86{,_64}
Sai Praneeth Prakhya (3):
x86/mm/pageattr: Introduce helper function to unmap EFI boot services
x86/efi: Unmap EFI boot services code/data regions from efi_pgd
x86/efi: Move efi_<reserve/free>_boot_services() to arch/x86
YiFei Zhu (1):
efi/x86: earlyprintk - Fix infinite loop on some screen widths
arch/x86/boot/compressed/eboot.c | 65 ++++++++++++++--------
arch/x86/include/asm/efi.h | 2 +
arch/x86/include/asm/pgtable_types.h | 8 ++-
arch/x86/mm/pageattr.c | 40 ++++++++++++-
arch/x86/platform/efi/early_printk.c | 2 +-
arch/x86/platform/efi/efi.c | 2 +
arch/x86/platform/efi/quirks.c | 25 +++++++++
drivers/firmware/efi/efi.c | 55 +++++++++++++-----
drivers/firmware/efi/libstub/Makefile | 5 +-
drivers/firmware/efi/libstub/arm-stub.c | 2 +-
drivers/firmware/efi/libstub/fdt.c | 30 +++++-----
drivers/firmware/efi/vars.c | 99 ++++++++++++++++++++++++++-------
include/linux/efi.h | 19 +++++--
init/main.c | 4 --
14 files changed, 269 insertions(+), 89 deletions(-)
From: Sai Praneeth Prakhya <[email protected]>
efi_<reserve/free>_boot_services() are x86 specific quirks and as such
should be in asm/efi.h, so move them from linux/efi.h. Also, call
efi_free_boot_services() from __efi_enter_virtual_mode() as it is x86
specific call and ideally shouldn't be part of init/main.c
Signed-off-by: Sai Praneeth Prakhya <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Acked-by: Thomas Gleixner <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/include/asm/efi.h | 2 ++
arch/x86/platform/efi/efi.c | 2 ++
include/linux/efi.h | 3 ---
init/main.c | 4 ----
4 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index eea40d52ca78..d1e64ac80b9c 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -141,6 +141,8 @@ 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);
extern void efi_recover_from_page_fault(unsigned long phys_addr);
+extern void efi_free_boot_services(void);
+extern void efi_reserve_boot_services(void);
struct efi_setup_data {
u64 fw_vendor;
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 7ae939e353cd..e1cb01a22fa8 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -993,6 +993,8 @@ static void __init __efi_enter_virtual_mode(void)
panic("EFI call to SetVirtualAddressMap() failed!");
}
+ efi_free_boot_services();
+
/*
* Now that EFI is in virtual mode, update the function
* pointers in the runtime service table to the new virtual addresses.
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 100ce4a4aff6..2b3b33c83b05 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1000,13 +1000,11 @@ extern void efi_memmap_walk (efi_freemem_callback_t callback, void *arg);
extern void efi_gettimeofday (struct timespec64 *ts);
extern void efi_enter_virtual_mode (void); /* switch EFI to virtual mode, if possible */
#ifdef CONFIG_X86
-extern void efi_free_boot_services(void);
extern efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size,
bool nonblocking);
extern void efi_find_mirror(void);
#else
-static inline void efi_free_boot_services(void) {}
static inline efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size,
@@ -1046,7 +1044,6 @@ extern void efi_mem_reserve(phys_addr_t addr, u64 size);
extern int efi_mem_reserve_persistent(phys_addr_t addr, u64 size);
extern void efi_initialize_iomem_resources(struct resource *code_resource,
struct resource *data_resource, struct resource *bss_resource);
-extern void efi_reserve_boot_services(void);
extern int efi_get_fdt_params(struct efi_fdt_params *params);
extern struct kobject *efi_kobj;
diff --git a/init/main.c b/init/main.c
index ee147103ba1b..ccefcd8e855f 100644
--- a/init/main.c
+++ b/init/main.c
@@ -737,10 +737,6 @@ asmlinkage __visible void __init start_kernel(void)
arch_post_acpi_subsys_init();
sfi_init_late();
- if (efi_enabled(EFI_RUNTIME_SERVICES)) {
- efi_free_boot_services();
- }
-
/* Do the rest non-__init'ed, we're now alive */
arch_call_rest_init();
}
--
2.19.1
From: YiFei Zhu <[email protected]>
An affected screen resolution is 1366 x 768, which width is not
divisible by 8, the default font width. On such screens, when longer
lines are earlyprintk'ed, overflow-to-next-line can never trigger,
due to the left-most x-coordinate of the next character always less
than the screen width. Earlyprintk will infinite loop in trying to
print the rest of the string but unable to, due to the line being
full.
This patch makes the trigger consider the right-most x-coordinate,
instead of left-most, as the value to compare against the screen
width threshold.
Signed-off-by: YiFei Zhu <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/platform/efi/early_printk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/platform/efi/early_printk.c b/arch/x86/platform/efi/early_printk.c
index 7476b3b097e1..7138bc7a265c 100644
--- a/arch/x86/platform/efi/early_printk.c
+++ b/arch/x86/platform/efi/early_printk.c
@@ -183,7 +183,7 @@ early_efi_write(struct console *con, const char *str, unsigned int num)
num--;
}
- if (efi_x >= si->lfb_width) {
+ if (efi_x + font->width > si->lfb_width) {
efi_x = 0;
efi_y += font->height;
}
--
2.19.1
From: Arend van Spriel <[email protected]>
Since commit:
ce2e6db554fa ("brcmfmac: Add support for getting nvram contents from
EFI variables")
we have a device driver accessing the efivars API. Several functions in
the efivars API assume __efivars is set, i.e., that they will be accessed
only after efivars_register() has been called. However, the following NULL
pointer access was reported calling efivar_entry_size() from the brcmfmac
device driver.
Unable to handle kernel NULL pointer dereference at virtual address 00000008
pgd = 60bfa5f1
[00000008] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
...
Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
Workqueue: events request_firmware_work_func
PC is at efivar_entry_size+0x28/0x90
LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
pc : [<c0c40718>] lr : [<bf2a3ef4>] psr: a00d0113
sp : ede7fe28 ip : ee983410 fp : c1787f30
r10: 00000000 r9 : 00000000 r8 : bf2b2258
r7 : ee983000 r6 : c1604c48 r5 : ede7fe88 r4 : edf337c0
r3 : 00000000 r2 : 00000000 r1 : ede7fe88 r0 : c17712c8
Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: ad16804a DAC: 00000051
Disassembly showed that the local static variable __efivars is NULL,
which is not entirely unexpected given that it is a non-EFI platform.
So add a NULL pointer check to efivar_entry_size(), and to related
functions while at it. In efivars_register() a couple of sanity checks
are added as well.
Cc: Hans de Goede <[email protected]>
Reported-by: Jon Hunter <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
drivers/firmware/efi/vars.c | 99 +++++++++++++++++++++++++++++--------
1 file changed, 78 insertions(+), 21 deletions(-)
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 9336ffdf6e2c..fceaafd67ec6 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -318,7 +318,12 @@ EXPORT_SYMBOL_GPL(efivar_variable_is_removable);
static efi_status_t
check_var_size(u32 attributes, unsigned long size)
{
- const struct efivar_operations *fops = __efivars->ops;
+ const struct efivar_operations *fops;
+
+ if (!__efivars)
+ return EFI_UNSUPPORTED;
+
+ fops = __efivars->ops;
if (!fops->query_variable_store)
return EFI_UNSUPPORTED;
@@ -329,7 +334,12 @@ check_var_size(u32 attributes, unsigned long size)
static efi_status_t
check_var_size_nonblocking(u32 attributes, unsigned long size)
{
- const struct efivar_operations *fops = __efivars->ops;
+ const struct efivar_operations *fops;
+
+ if (!__efivars)
+ return EFI_UNSUPPORTED;
+
+ fops = __efivars->ops;
if (!fops->query_variable_store)
return EFI_UNSUPPORTED;
@@ -429,13 +439,18 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
void *data, bool duplicates, struct list_head *head)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
unsigned long variable_name_size = 1024;
efi_char16_t *variable_name;
efi_status_t status;
efi_guid_t vendor_guid;
int err = 0;
+ if (!__efivars)
+ return -EFAULT;
+
+ ops = __efivars->ops;
+
variable_name = kzalloc(variable_name_size, GFP_KERNEL);
if (!variable_name) {
printk(KERN_ERR "efivars: Memory allocation failed.\n");
@@ -583,12 +598,14 @@ static void efivar_entry_list_del_unlock(struct efivar_entry *entry)
*/
int __efivar_entry_delete(struct efivar_entry *entry)
{
- const struct efivar_operations *ops = __efivars->ops;
efi_status_t status;
- status = ops->set_variable(entry->var.VariableName,
- &entry->var.VendorGuid,
- 0, 0, NULL);
+ if (!__efivars)
+ return -EINVAL;
+
+ status = __efivars->ops->set_variable(entry->var.VariableName,
+ &entry->var.VendorGuid,
+ 0, 0, NULL);
return efi_status_to_err(status);
}
@@ -607,12 +624,17 @@ EXPORT_SYMBOL_GPL(__efivar_entry_delete);
*/
int efivar_entry_delete(struct efivar_entry *entry)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_status_t status;
if (down_interruptible(&efivars_lock))
return -EINTR;
+ if (!__efivars) {
+ up(&efivars_lock);
+ return -EINVAL;
+ }
+ ops = __efivars->ops;
status = ops->set_variable(entry->var.VariableName,
&entry->var.VendorGuid,
0, 0, NULL);
@@ -650,13 +672,19 @@ EXPORT_SYMBOL_GPL(efivar_entry_delete);
int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
unsigned long size, void *data, struct list_head *head)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_status_t status;
efi_char16_t *name = entry->var.VariableName;
efi_guid_t vendor = entry->var.VendorGuid;
if (down_interruptible(&efivars_lock))
return -EINTR;
+
+ if (!__efivars) {
+ up(&efivars_lock);
+ return -EINVAL;
+ }
+ ops = __efivars->ops;
if (head && efivar_entry_find(name, vendor, head, false)) {
up(&efivars_lock);
return -EEXIST;
@@ -687,12 +715,17 @@ static int
efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
u32 attributes, unsigned long size, void *data)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_status_t status;
if (down_trylock(&efivars_lock))
return -EBUSY;
+ if (!__efivars) {
+ up(&efivars_lock);
+ return -EINVAL;
+ }
+
status = check_var_size_nonblocking(attributes,
size + ucs2_strsize(name, 1024));
if (status != EFI_SUCCESS) {
@@ -700,6 +733,7 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
return -ENOSPC;
}
+ ops = __efivars->ops;
status = ops->set_variable_nonblocking(name, &vendor, attributes,
size, data);
@@ -727,9 +761,13 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
bool block, unsigned long size, void *data)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_status_t status;
+ if (!__efivars)
+ return -EINVAL;
+
+ ops = __efivars->ops;
if (!ops->query_variable_store)
return -ENOSYS;
@@ -829,13 +867,18 @@ EXPORT_SYMBOL_GPL(efivar_entry_find);
*/
int efivar_entry_size(struct efivar_entry *entry, unsigned long *size)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_status_t status;
*size = 0;
if (down_interruptible(&efivars_lock))
return -EINTR;
+ if (!__efivars) {
+ up(&efivars_lock);
+ return -EINVAL;
+ }
+ ops = __efivars->ops;
status = ops->get_variable(entry->var.VariableName,
&entry->var.VendorGuid, NULL, size, NULL);
up(&efivars_lock);
@@ -861,12 +904,14 @@ EXPORT_SYMBOL_GPL(efivar_entry_size);
int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
unsigned long *size, void *data)
{
- const struct efivar_operations *ops = __efivars->ops;
efi_status_t status;
- status = ops->get_variable(entry->var.VariableName,
- &entry->var.VendorGuid,
- attributes, size, data);
+ if (!__efivars)
+ return -EINVAL;
+
+ status = __efivars->ops->get_variable(entry->var.VariableName,
+ &entry->var.VendorGuid,
+ attributes, size, data);
return efi_status_to_err(status);
}
@@ -882,14 +927,19 @@ EXPORT_SYMBOL_GPL(__efivar_entry_get);
int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
unsigned long *size, void *data)
{
- const struct efivar_operations *ops = __efivars->ops;
efi_status_t status;
if (down_interruptible(&efivars_lock))
return -EINTR;
- status = ops->get_variable(entry->var.VariableName,
- &entry->var.VendorGuid,
- attributes, size, data);
+
+ if (!__efivars) {
+ up(&efivars_lock);
+ return -EINVAL;
+ }
+
+ status = __efivars->ops->get_variable(entry->var.VariableName,
+ &entry->var.VendorGuid,
+ attributes, size, data);
up(&efivars_lock);
return efi_status_to_err(status);
@@ -921,7 +971,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_get);
int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
unsigned long *size, void *data, bool *set)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_char16_t *name = entry->var.VariableName;
efi_guid_t *vendor = &entry->var.VendorGuid;
efi_status_t status;
@@ -940,6 +990,11 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
if (down_interruptible(&efivars_lock))
return -EINTR;
+ if (!__efivars) {
+ err = -EINVAL;
+ goto out;
+ }
+
/*
* Ensure that the available space hasn't shrunk below the safe level
*/
@@ -956,6 +1011,8 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
}
}
+ ops = __efivars->ops;
+
status = ops->set_variable(name, vendor, attributes, *size, data);
if (status != EFI_SUCCESS) {
err = efi_status_to_err(status);
--
2.19.1
The current implementation of efi_mem_reserve_persistent() is rather
naive, in the sense that for each invocation, it creates a separate
linked list entry to describe the reservation. Since the linked list
entries themselves need to persist across subsequent kexec reboots,
every reservation created this way results in two memblock_reserve()
calls at the next boot.
On arm64 systems with 100s of CPUs, this may result in a excessive
number of memblock reservations, and needless fragmentation.
So instead, make use of the newly updated struct linux_efi_memreserve
layout to put multiple reservations into a single linked list entry.
This should get rid of the numerous tiny memblock reservations, and
effectively cut the total number of reservations in half on arm64
systems with many CPUs.
Tested-by: Marc Zyngier <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
drivers/firmware/efi/efi.c | 20 +++++++++++++++++---
include/linux/efi.h | 3 +++
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 80b11521627a..e90bc32c2670 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -998,7 +998,8 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
{
struct linux_efi_memreserve *rsv;
int rsvsize = EFI_MEMRESERVE_SIZE(1);
- int rc;
+ unsigned long prsv;
+ int rc, index;
if (efi_memreserve_root == (void *)ULONG_MAX)
return -ENODEV;
@@ -1009,11 +1010,24 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
return rc;
}
- rsv = kmalloc(rsvsize, GFP_ATOMIC);
+ /* first try to find a slot in an existing linked list entry */
+ for (prsv = efi_memreserve_root->next; prsv; prsv = rsv->next) {
+ rsv = __va(prsv);
+ index = atomic_fetch_add_unless(&rsv->count, 1, rsv->size);
+ if (index < rsv->size) {
+ rsv->entry[index].base = addr;
+ rsv->entry[index].size = size;
+
+ return 0;
+ }
+ }
+
+ /* no slot found - allocate a new linked list entry */
+ rsv = (struct linux_efi_memreserve *)__get_free_page(GFP_ATOMIC);
if (!rsv)
return -ENOMEM;
- rsv->size = 1;
+ rsv->size = EFI_MEMRESERVE_COUNT(PAGE_SIZE);
atomic_set(&rsv->count, 1);
rsv->entry[0].base = addr;
rsv->entry[0].size = size;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 4f27640fdcdc..becd5d76a207 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1724,4 +1724,7 @@ struct linux_efi_memreserve {
#define EFI_MEMRESERVE_SIZE(count) (sizeof(struct linux_efi_memreserve) + \
(count) * sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
+#define EFI_MEMRESERVE_COUNT(size) (((size) - sizeof(struct linux_efi_memreserve)) \
+ / sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
+
#endif /* _LINUX_EFI_H */
--
2.19.1
In preparation of updating efi_mem_reserve_persistent() to cause less
fragmentation when dealing with many persistent reservations, update
the struct definition and the code that handles it currently so it
can describe an arbitrary number of reservations using a single linked
list entry. The actual optimization will be implemented in a subsequent
patch.
Tested-by: Marc Zyngier <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
drivers/firmware/efi/efi.c | 39 +++++++++++++++++--------
drivers/firmware/efi/libstub/arm-stub.c | 2 +-
include/linux/efi.h | 13 +++++++--
3 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 415849bab233..80b11521627a 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -602,21 +602,33 @@ int __init efi_apply_persistent_mem_reservations(void)
while (prsv) {
struct linux_efi_memreserve *rsv;
-
- /* reserve the entry itself */
- memblock_reserve(prsv, sizeof(*rsv));
-
- rsv = early_memremap(prsv, sizeof(*rsv));
- if (rsv == NULL) {
+ u8 *p;
+ int i;
+
+ /*
+ * Just map a full page: that is what we will get
+ * anyway, and it permits us to map the entire entry
+ * before knowing its size.
+ */
+ p = early_memremap(ALIGN_DOWN(prsv, PAGE_SIZE),
+ PAGE_SIZE);
+ if (p == NULL) {
pr_err("Could not map UEFI memreserve entry!\n");
return -ENOMEM;
}
- if (rsv->size)
- memblock_reserve(rsv->base, rsv->size);
+ rsv = (void *)(p + prsv % PAGE_SIZE);
+
+ /* reserve the entry itself */
+ memblock_reserve(prsv, EFI_MEMRESERVE_SIZE(rsv->size));
+
+ for (i = 0; i < atomic_read(&rsv->count); i++) {
+ memblock_reserve(rsv->entry[i].base,
+ rsv->entry[i].size);
+ }
prsv = rsv->next;
- early_memunmap(rsv, sizeof(*rsv));
+ early_memunmap(p, PAGE_SIZE);
}
}
@@ -985,6 +997,7 @@ static int __init efi_memreserve_map_root(void)
int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
{
struct linux_efi_memreserve *rsv;
+ int rsvsize = EFI_MEMRESERVE_SIZE(1);
int rc;
if (efi_memreserve_root == (void *)ULONG_MAX)
@@ -996,12 +1009,14 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
return rc;
}
- rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
+ rsv = kmalloc(rsvsize, GFP_ATOMIC);
if (!rsv)
return -ENOMEM;
- rsv->base = addr;
- rsv->size = size;
+ rsv->size = 1;
+ atomic_set(&rsv->count, 1);
+ rsv->entry[0].base = addr;
+ rsv->entry[0].size = size;
spin_lock(&efi_mem_reserve_persistent_lock);
rsv->next = efi_memreserve_root->next;
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 3d36142cf812..9e20159ea5f5 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -86,8 +86,8 @@ void install_memreserve_table(efi_system_table_t *sys_table_arg)
}
rsv->next = 0;
- rsv->base = 0;
rsv->size = 0;
+ atomic_set(&rsv->count, 0);
status = efi_call_early(install_configuration_table,
&memreserve_table_guid,
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2b3b33c83b05..4f27640fdcdc 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1712,9 +1712,16 @@ extern struct efi_runtime_work efi_rts_work;
extern struct workqueue_struct *efi_rts_wq;
struct linux_efi_memreserve {
- phys_addr_t next;
- phys_addr_t base;
- phys_addr_t size;
+ int size; // allocated size of the array
+ atomic_t count; // number of entries used
+ phys_addr_t next; // pa of next struct instance
+ struct {
+ phys_addr_t base;
+ phys_addr_t size;
+ } entry[0];
};
+#define EFI_MEMRESERVE_SIZE(count) (sizeof(struct linux_efi_memreserve) + \
+ (count) * sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
+
#endif /* _LINUX_EFI_H */
--
2.19.1
From: Julien Thierry <[email protected]>
Closing bracket seems to end a for statement when it is actually ending
the contained if. Add some brackets to have clear delimitation of each
scope.
No functional change/fix, just fix the indentation.
Signed-off-by: Julien Thierry <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
drivers/firmware/efi/libstub/fdt.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 0c0d2312f4a8..a3614f9b5f75 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -376,7 +376,7 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
tables = (efi_config_table_t *) sys_table->tables;
fdt = NULL;
- for (i = 0; i < sys_table->nr_tables; i++)
+ for (i = 0; i < sys_table->nr_tables; i++) {
if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
fdt = (void *) tables[i].table;
if (fdt_check_header(fdt) != 0) {
@@ -385,7 +385,8 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
}
*fdt_size = fdt_totalsize(fdt);
break;
- }
+ }
+ }
return fdt;
}
--
2.19.1
From: Sai Praneeth Prakhya <[email protected]>
Ideally, after kernel assumes control of the platform, firmware
shouldn't access EFI boot services code/data regions. But, it's noticed
that this is not so true in many x86 platforms. Hence, during boot,
kernel reserves EFI boot services code/data regions [1] and maps [2]
them to efi_pgd so that call to set_virtual_address_map() doesn't fail.
After returning from set_virtual_address_map(), kernel frees the
reserved regions [3] but they still remain mapped. Hence, introduce
kernel_unmap_pages_in_pgd() which will later be used to unmap EFI boot
services code/data regions.
While at it modify kernel_map_pages_in_pgd() by
1. Adding __init modifier because it's always used *only* during boot.
2. Add a warning if it's used after SMP is initialized because it uses
__flush_tlb_all() which flushes mappings only on current CPU.
Unmapping EFI boot services code/data regions will result in clearing
PAGE_PRESENT bit and it shouldn't bother L1TF cases because it's already
handled by protnone_mask() at arch/x86/include/asm/pgtable-invert.h.
[1] efi_reserve_boot_services()
[2] efi_map_region() -> __map_region() -> kernel_map_pages_in_pgd()
[3] efi_free_boot_services()
Signed-off-by: Sai Praneeth Prakhya <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/include/asm/pgtable_types.h | 8 ++++--
arch/x86/mm/pageattr.c | 40 ++++++++++++++++++++++++++--
2 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 106b7d0e2dae..d6ff0bbdb394 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -564,8 +564,12 @@ extern pte_t *lookup_address_in_pgd(pgd_t *pgd, unsigned long address,
unsigned int *level);
extern pmd_t *lookup_pmd_address(unsigned long address);
extern phys_addr_t slow_virt_to_phys(void *__address);
-extern int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
- unsigned numpages, unsigned long page_flags);
+extern int __init kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn,
+ unsigned long address,
+ unsigned numpages,
+ unsigned long page_flags);
+extern int __init kernel_unmap_pages_in_pgd(pgd_t *pgd, unsigned long address,
+ unsigned long numpages);
#endif /* !__ASSEMBLY__ */
#endif /* _ASM_X86_PGTABLE_DEFS_H */
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index db7a10082238..bac35001d896 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -2338,8 +2338,8 @@ bool kernel_page_present(struct page *page)
#endif /* CONFIG_DEBUG_PAGEALLOC */
-int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
- unsigned numpages, unsigned long page_flags)
+int __init kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
+ unsigned numpages, unsigned long page_flags)
{
int retval = -EINVAL;
@@ -2353,6 +2353,8 @@ int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
.flags = 0,
};
+ WARN_ONCE(num_online_cpus() > 1, "Don't call after initializing SMP");
+
if (!(__supported_pte_mask & _PAGE_NX))
goto out;
@@ -2374,6 +2376,40 @@ int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
return retval;
}
+/*
+ * __flush_tlb_all() flushes mappings only on current CPU and hence this
+ * function shouldn't be used in an SMP environment. Presently, it's used only
+ * during boot (way before smp_init()) by EFI subsystem and hence is ok.
+ */
+int __init kernel_unmap_pages_in_pgd(pgd_t *pgd, unsigned long address,
+ unsigned long numpages)
+{
+ int retval;
+
+ /*
+ * The typical sequence for unmapping is to find a pte through
+ * lookup_address_in_pgd() (ideally, it should never return NULL because
+ * the address is already mapped) and change it's protections. As pfn is
+ * the *target* of a mapping, it's not useful while unmapping.
+ */
+ struct cpa_data cpa = {
+ .vaddr = &address,
+ .pfn = 0,
+ .pgd = pgd,
+ .numpages = numpages,
+ .mask_set = __pgprot(0),
+ .mask_clr = __pgprot(_PAGE_PRESENT | _PAGE_RW),
+ .flags = 0,
+ };
+
+ WARN_ONCE(num_online_cpus() > 1, "Don't call after initializing SMP");
+
+ retval = __change_page_attr_set_clr(&cpa, 0);
+ __flush_tlb_all();
+
+ return retval;
+}
+
/*
* The testcases use internal knowledge of the implementation that shouldn't
* be exposed to the rest of the kernel. Include these directly here.
--
2.19.1
From: Eric Snowberg <[email protected]>
Commit d64934019f6c ("x86/efi: Use efi_exit_boot_services()")
introduced a regression on systems with large memory maps
causing them to hang on boot. The first "goto get_map" that was removed
from exit_boot insured there was enough room for the memory map when
efi_call_early(exit_boot_services) was called. This happens when
(nr_desc > ARRAY_SIZE(params->e820_table).
Chain of events:
exit_boot()
efi_exit_boot_services()
efi_get_memory_map <- at this point the mm can't grow over 8 desc
priv_func()
exit_boot_func()
allocate_e820ext() <- new mm grows over 8 desc from e820 alloc
efi_call_early(exit_boot_services) <- mm key doesn't match so retry
efi_call_early(get_memory_map) <- not enough room for new mm
system hangs
This patch allocates the e820 buffer before calling efi_exit_boot_services
and fixes the regression.
Signed-off-by: Eric Snowberg <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/boot/compressed/eboot.c | 65 ++++++++++++++++++++------------
1 file changed, 41 insertions(+), 24 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 8b4c5e001157..f7bad07bb251 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1,3 +1,4 @@
+
/* -----------------------------------------------------------------------
*
* Copyright 2011 Intel Corporation; author Matt Fleming
@@ -634,37 +635,54 @@ static efi_status_t alloc_e820ext(u32 nr_desc, struct setup_data **e820ext,
return status;
}
+static efi_status_t allocate_e820(struct boot_params *params,
+ struct setup_data **e820ext,
+ u32 *e820ext_size)
+{
+ unsigned long map_size, desc_size, buff_size;
+ struct efi_boot_memmap boot_map;
+ efi_memory_desc_t *map;
+ efi_status_t status;
+ __u32 nr_desc;
+
+ boot_map.map = ↦
+ boot_map.map_size = &map_size;
+ boot_map.desc_size = &desc_size;
+ boot_map.desc_ver = NULL;
+ boot_map.key_ptr = NULL;
+ boot_map.buff_size = &buff_size;
+
+ status = efi_get_memory_map(sys_table, &boot_map);
+ if (status != EFI_SUCCESS)
+ return status;
+
+ nr_desc = buff_size / desc_size;
+
+ if (nr_desc > ARRAY_SIZE(params->e820_table)) {
+ u32 nr_e820ext = nr_desc - ARRAY_SIZE(params->e820_table);
+
+ status = alloc_e820ext(nr_e820ext, e820ext, e820ext_size);
+ if (status != EFI_SUCCESS)
+ return status;
+ }
+
+ return EFI_SUCCESS;
+}
+
struct exit_boot_struct {
struct boot_params *boot_params;
struct efi_info *efi;
- struct setup_data *e820ext;
- __u32 e820ext_size;
};
static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
struct efi_boot_memmap *map,
void *priv)
{
- static bool first = true;
const char *signature;
__u32 nr_desc;
efi_status_t status;
struct exit_boot_struct *p = priv;
- if (first) {
- nr_desc = *map->buff_size / *map->desc_size;
- if (nr_desc > ARRAY_SIZE(p->boot_params->e820_table)) {
- u32 nr_e820ext = nr_desc -
- ARRAY_SIZE(p->boot_params->e820_table);
-
- status = alloc_e820ext(nr_e820ext, &p->e820ext,
- &p->e820ext_size);
- if (status != EFI_SUCCESS)
- return status;
- }
- first = false;
- }
-
signature = efi_is_64bit() ? EFI64_LOADER_SIGNATURE
: EFI32_LOADER_SIGNATURE;
memcpy(&p->efi->efi_loader_signature, signature, sizeof(__u32));
@@ -687,8 +705,8 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
{
unsigned long map_sz, key, desc_size, buff_size;
efi_memory_desc_t *mem_map;
- struct setup_data *e820ext;
- __u32 e820ext_size;
+ struct setup_data *e820ext = NULL;
+ __u32 e820ext_size = 0;
efi_status_t status;
__u32 desc_version;
struct efi_boot_memmap map;
@@ -702,8 +720,10 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
map.buff_size = &buff_size;
priv.boot_params = boot_params;
priv.efi = &boot_params->efi_info;
- priv.e820ext = NULL;
- priv.e820ext_size = 0;
+
+ status = allocate_e820(boot_params, &e820ext, &e820ext_size);
+ if (status != EFI_SUCCESS)
+ return status;
/* Might as well exit boot services now */
status = efi_exit_boot_services(sys_table, handle, &map, &priv,
@@ -711,9 +731,6 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
if (status != EFI_SUCCESS)
return status;
- e820ext = priv.e820ext;
- e820ext_size = priv.e820ext_size;
-
/* Historic? */
boot_params->alt_mem_k = 32 * 1024;
--
2.19.1
From: Sai Praneeth Prakhya <[email protected]>
efi_free_boot_services(), as the name suggests, frees EFI boot services
code/data regions but forgets to unmap these regions from efi_pgd. This
means that any code that's running in efi_pgd address space (e.g:
any EFI runtime service) would still be able to access these regions but
the contents of these regions would have long been over written by
someone else. So, it's important to unmap these regions. Hence,
introduce efi_unmap_pages() to unmap these regions from efi_pgd.
After unmapping EFI boot services code/data regions, any illegal access
by buggy firmware to these regions would result in page fault which will
be handled by EFI specific fault handler.
Signed-off-by: Sai Praneeth Prakhya <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Acked-by: Thomas Gleixner <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
arch/x86/platform/efi/quirks.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 95e77a667ba5..09e811b9da26 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -369,6 +369,24 @@ void __init efi_reserve_boot_services(void)
}
}
+/*
+ * Apart from having VA mappings for EFI boot services code/data regions,
+ * (duplicate) 1:1 mappings were also created as a quirk for buggy firmware. So,
+ * unmap both 1:1 and VA mappings.
+ */
+static void __init efi_unmap_pages(efi_memory_desc_t *md)
+{
+ pgd_t *pgd = efi_mm.pgd;
+ u64 pa = md->phys_addr;
+ u64 va = md->virt_addr;
+
+ if (kernel_unmap_pages_in_pgd(pgd, pa, md->num_pages))
+ pr_err("Failed to unmap 1:1 mapping for 0x%llx\n", pa);
+
+ if (kernel_unmap_pages_in_pgd(pgd, va, md->num_pages))
+ pr_err("Failed to unmap VA mapping for 0x%llx\n", va);
+}
+
void __init efi_free_boot_services(void)
{
phys_addr_t new_phys, new_size;
@@ -393,6 +411,13 @@ void __init efi_free_boot_services(void)
continue;
}
+ /*
+ * Before calling set_virtual_address_map(), EFI boot services
+ * code/data regions were mapped as a quirk for buggy firmware.
+ * Unmap them from efi_pgd before freeing them up.
+ */
+ efi_unmap_pages(md);
+
/*
* Nasty quirk: if all sub-1MB memory is used for boot
* services, we can get here without having allocated the
--
2.19.1
From: Julien Thierry <[email protected]>
Reorganize get_fdt lookup loop, clearly showing that:
- Nothing is done for table entries that do not have fdt_guid
- Once an entry with fdt_guid is found, break out of the loop
No functional changes.
Suggested-by: Joe Perches <[email protected]>
Signed-off-by: Julien Thierry <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
drivers/firmware/efi/libstub/fdt.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index a3614f9b5f75..0dc7b4987cc2 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -370,23 +370,24 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
{
efi_guid_t fdt_guid = DEVICE_TREE_GUID;
efi_config_table_t *tables;
- void *fdt;
int i;
- tables = (efi_config_table_t *) sys_table->tables;
- fdt = NULL;
+ tables = (efi_config_table_t *)sys_table->tables;
for (i = 0; i < sys_table->nr_tables; i++) {
- if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
- fdt = (void *) tables[i].table;
- if (fdt_check_header(fdt) != 0) {
- pr_efi_err(sys_table, "Invalid header detected on UEFI supplied FDT, ignoring ...\n");
- return NULL;
- }
- *fdt_size = fdt_totalsize(fdt);
- break;
+ void *fdt;
+
+ if (efi_guidcmp(tables[i].guid, fdt_guid) != 0)
+ continue;
+
+ fdt = (void *)tables[i].table;
+ if (fdt_check_header(fdt) != 0) {
+ pr_efi_err(sys_table, "Invalid header detected on UEFI supplied FDT, ignoring ...\n");
+ return NULL;
}
+ *fdt_size = fdt_totalsize(fdt);
+ return fdt;
}
- return fdt;
+ return NULL;
}
--
2.19.1
From: Nathan Chancellor <[email protected]>
When building the kernel with Clang, some disabled warnings appear
because this Makefile overrides KBUILD_CFLAGS for x86{,_64}. Add them to
this list so that the build is clean again.
-Wpointer-sign was disabled for the whole kernel before the beginning
of git history.
-Waddress-of-packed-member was disabled for the whole kernel in
commit bfb38988c51e ("kbuild: clang: Disable 'address-of-packed-member'
warning") and for x86/boot/compressed in commit 20c6c1890455 ("x86/boot:
Disable the address-of-packed-member compiler warning").
-Wgnu was disabled for the whole kernel in commit 61163efae020 ("kbuild:
LLVMLinux: Add Kbuild support for building kernel with Clang") and for
x86/boot/compressed in commit 6c3b56b19730 ("x86/boot: Disable Clang
warnings about GNU extensions").
Link: https://github.com/ClangBuiltLinux/linux/issues/112
Signed-off-by: Nathan Chancellor <[email protected]>
Reviewed-by: Sedat Dilek <[email protected]>
Tested-by: Sedat Dilek <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
drivers/firmware/efi/libstub/Makefile | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index c51627660dbb..d9845099635e 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -9,7 +9,10 @@ cflags-$(CONFIG_X86_32) := -march=i386
cflags-$(CONFIG_X86_64) := -mcmodel=small
cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ -O2 \
-fPIC -fno-strict-aliasing -mno-red-zone \
- -mno-mmx -mno-sse -fshort-wchar
+ -mno-mmx -mno-sse -fshort-wchar \
+ -Wno-pointer-sign \
+ $(call cc-disable-warning, address-of-packed-member) \
+ $(call cc-disable-warning, gnu)
# arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly
# disable the stackleak plugin
--
2.19.1
Hi Ard,
While building from next branch of efi tree, I noticed the below warning. Could you please check the same on your side?
CC lib/list_debug.o
drivers/firmware/efi/efi.c: In function 'efi_mem_reserve_persistent':
drivers/firmware/efi/efi.c:1000:6: warning: unused variable 'rsvsize' [-Wunused-variable]
int rsvsize = EFI_MEMRESERVE_SIZE(1);
^~~~~~~
CC lib/bitrev.o
CC net/core/sock_reuseport.o
> -----Original Message-----
> From: Ard Biesheuvel [mailto:[email protected]]
> Sent: Thursday, November 29, 2018 9:12 AM
> To: [email protected]; Ingo Molnar <[email protected]>; Thomas
> Gleixner <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>; [email protected];
> Andy Lutomirski <[email protected]>; Arend van Spriel
> <[email protected]>; Bhupesh Sharma <[email protected]>;
> Borislav Petkov <[email protected]>; Hansen, Dave <[email protected]>; Eric
> Snowberg <[email protected]>; Hans de Goede
> <[email protected]>; Joe Perches <[email protected]>; Jon Hunter
> <[email protected]>; Julien Thierry <[email protected]>; Marc
> Zyngier <[email protected]>; Nathan Chancellor
> <[email protected]>; Peter Zijlstra <[email protected]>; Prakhya,
> Sai Praneeth <[email protected]>; Sedat Dilek
> <[email protected]>; YiFei Zhu <[email protected]>
> Subject: [GIT PULL 00/11] EFI updates
>
> The following changes since commit
> 976b489120cdab2b1b3a41ffa14661db43d58190:
>
> efi: Prevent GICv3 WARN() by mapping the memreserve table before first use
> (2018-11-27 13:50:20 +0100)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-next
>
> for you to fetch changes up to 1d29afdbf7ae878a23627ebee81efcd213f11749:
>
> efi/x86: earlyprintk - Fix infinite loop on some screen widths (2018-11-28
> 17:58:42 +0100)
>
Regards,
Sai
* Ard Biesheuvel <[email protected]> wrote:
> From: Eric Snowberg <[email protected]>
>
> Commit d64934019f6c ("x86/efi: Use efi_exit_boot_services()")
> introduced a regression on systems with large memory maps
> causing them to hang on boot. The first "goto get_map" that was removed
> from exit_boot insured there was enough room for the memory map when
> efi_call_early(exit_boot_services) was called. This happens when
> (nr_desc > ARRAY_SIZE(params->e820_table).
>
> Chain of events:
> exit_boot()
> efi_exit_boot_services()
> efi_get_memory_map <- at this point the mm can't grow over 8 desc
> priv_func()
> exit_boot_func()
> allocate_e820ext() <- new mm grows over 8 desc from e820 alloc
> efi_call_early(exit_boot_services) <- mm key doesn't match so retry
> efi_call_early(get_memory_map) <- not enough room for new mm
> system hangs
>
> This patch allocates the e820 buffer before calling efi_exit_boot_services
> and fixes the regression.
>
> Signed-off-by: Eric Snowberg <[email protected]>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> arch/x86/boot/compressed/eboot.c | 65 ++++++++++++++++++++------------
> 1 file changed, 41 insertions(+), 24 deletions(-)
Any objections against marking this for -stable and filing it in
efi/urgent? Boot hangs are show-stopper bugs, so distributions would want
to backport this fix anyway.
> + boot_map.map = ↦
> + boot_map.map_size = &map_size;
> + boot_map.desc_size = &desc_size;
> + boot_map.desc_ver = NULL;
> + boot_map.key_ptr = NULL;
> + boot_map.buff_size = &buff_size;
I have changed this to the canonical style, which is also used elsewhere
in the file:
> map.buff_size = &buff_size;
> priv.boot_params = boot_params;
> priv.efi = &boot_params->efi_info;
> - priv.e820ext = NULL;
> - priv.e820ext_size = 0;
I.e.:
+ boot_map.map = ↦
+ boot_map.map_size = &map_size;
+ boot_map.desc_size = &desc_size;
+ boot_map.desc_ver = NULL;
+ boot_map.key_ptr = NULL;
+ boot_map.buff_size = &buff_size;
+
+ status = efi_get_memory_map(sys_table, &boot_map);
Thanks,
Ingo
* Ard Biesheuvel <[email protected]> wrote:
> From: Julien Thierry <[email protected]>
>
> Closing bracket seems to end a for statement when it is actually ending
> the contained if. Add some brackets to have clear delimitation of each
> scope.
>
> No functional change/fix, just fix the indentation.
>
> Signed-off-by: Julien Thierry <[email protected]>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> drivers/firmware/efi/libstub/fdt.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 0c0d2312f4a8..a3614f9b5f75 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -376,7 +376,7 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
> tables = (efi_config_table_t *) sys_table->tables;
> fdt = NULL;
>
> - for (i = 0; i < sys_table->nr_tables; i++)
> + for (i = 0; i < sys_table->nr_tables; i++) {
> if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
> fdt = (void *) tables[i].table;
> if (fdt_check_header(fdt) != 0) {
> @@ -385,7 +385,8 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
> }
> *fdt_size = fdt_totalsize(fdt);
> break;
> - }
> + }
> + }
So if we are doing trivial cleanups, how about the patch below on top? It
cleans up this file for good. Only minimally build tested.
Thanks,
Ingo
======================>
Subject: efi/fdt: More cleanups
From: Ingo Molnar <[email protected]>
Apply a number of cleanups:
- Introduce fdt_setprop_*var() helper macros to simplify and shorten repetitive
sequences - this also makes it less likely that the wrong variable size is
passed in. This change makes a lot of the property-setting calls single-line
and easier to read.
- Harmonize comment style: capitalization, punctuation, whitespaces, etc.
- Fix some whitespace noise in the libstub Makefile which I happened to notice.
- Use the standard tabular initialization style:
- map.map = &runtime_map;
- map.map_size = &map_size;
- map.desc_size = &desc_size;
- map.desc_ver = &desc_ver;
- map.key_ptr = &mmap_key;
- map.buff_size = &buff_size;
+ map.map = &runtime_map;
+ map.map_size = &map_size;
+ map.desc_size = &desc_size;
+ map.desc_ver = &desc_ver;
+ map.key_ptr = &mmap_key;
+ map.buff_size = &buff_size;
- Use tabular structure definition for better readability.
- Make all pr*() lines single-line, even if they marginally exceed 80 cols - this
makes them visually less intrusive.
- Unbreak line breaks into single lines when the length exceeds 80 cols only
marginally, for better readability.
- Move assignment closer to the actual usage site.
- Plus some other smaller cleanups, spelling fixes, etc.
No change in functionality intended.
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
drivers/firmware/efi/libstub/Makefile | 4 -
drivers/firmware/efi/libstub/fdt.c | 109 ++++++++++++++++------------------
scripts/dtc/libfdt/libfdt.h | 10 +++
3 files changed, 64 insertions(+), 59 deletions(-)
Index: tip/drivers/firmware/efi/libstub/Makefile
===================================================================
--- tip.orig/drivers/firmware/efi/libstub/Makefile
+++ tip/drivers/firmware/efi/libstub/Makefile
@@ -49,7 +49,7 @@ lib-$(CONFIG_EFI_ARMSTUB) += arm-stub.o
lib-$(CONFIG_ARM) += arm32-stub.o
lib-$(CONFIG_ARM64) += arm64-stub.o
-CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
+CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
#
# arm64 puts the stub in the kernel proper, which will unnecessarily retain all
@@ -86,7 +86,7 @@ quiet_cmd_stubcopy = STUBCPY $@
cmd_stubcopy = if $(STRIP) --strip-debug $(STUBCOPY_RM-y) -o $@ $<; \
then if $(OBJDUMP) -r $@ | grep $(STUBCOPY_RELOC-y); \
then (echo >&2 "$@: absolute symbol references not allowed in the EFI stub"; \
- rm -f $@; /bin/false); \
+ rm -f $@; /bin/false); \
else $(OBJCOPY) $(STUBCOPY_FLAGS-y) $< $@; fi \
else /bin/false; fi
Index: tip/drivers/firmware/efi/libstub/fdt.c
===================================================================
--- tip.orig/drivers/firmware/efi/libstub/fdt.c
+++ tip/drivers/firmware/efi/libstub/fdt.c
@@ -26,10 +26,8 @@ static void fdt_update_cell_size(efi_sys
offset = fdt_path_offset(fdt, "/");
/* Set the #address-cells and #size-cells values for an empty tree */
- fdt_setprop_u32(fdt, offset, "#address-cells",
- EFI_DT_ADDR_CELLS_DEFAULT);
-
- fdt_setprop_u32(fdt, offset, "#size-cells", EFI_DT_SIZE_CELLS_DEFAULT);
+ fdt_setprop_u32(fdt, offset, "#address-cells", EFI_DT_ADDR_CELLS_DEFAULT);
+ fdt_setprop_u32(fdt, offset, "#size-cells", EFI_DT_SIZE_CELLS_DEFAULT);
}
static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
@@ -42,7 +40,7 @@ static efi_status_t update_fdt(efi_syste
u32 fdt_val32;
u64 fdt_val64;
- /* Do some checks on provided FDT, if it exists*/
+ /* Do some checks on provided FDT, if it exists: */
if (orig_fdt) {
if (fdt_check_header(orig_fdt)) {
pr_efi_err(sys_table, "Device Tree header not valid!\n");
@@ -50,7 +48,7 @@ static efi_status_t update_fdt(efi_syste
}
/*
* We don't get the size of the FDT if we get if from a
- * configuration table.
+ * configuration table:
*/
if (orig_fdt_size && fdt_totalsize(orig_fdt) > orig_fdt_size) {
pr_efi_err(sys_table, "Truncated device tree! foo!\n");
@@ -64,8 +62,8 @@ static efi_status_t update_fdt(efi_syste
status = fdt_create_empty_tree(fdt, new_fdt_size);
if (status == 0) {
/*
- * Any failure from the following function is non
- * critical
+ * Any failure from the following function is
+ * non-critical:
*/
fdt_update_cell_size(sys_table, fdt);
}
@@ -86,12 +84,13 @@ static efi_status_t update_fdt(efi_syste
if (node < 0) {
node = fdt_add_subnode(fdt, 0, "chosen");
if (node < 0) {
- status = node; /* node is error code when negative */
+ /* 'node' is an error code when negative: */
+ status = node;
goto fdt_set_fail;
}
}
- if ((cmdline_ptr != NULL) && (strlen(cmdline_ptr) > 0)) {
+ if (cmdline_ptr != NULL && strlen(cmdline_ptr) > 0) {
status = fdt_setprop(fdt, node, "bootargs", cmdline_ptr,
strlen(cmdline_ptr) + 1);
if (status)
@@ -103,13 +102,12 @@ static efi_status_t update_fdt(efi_syste
u64 initrd_image_end;
u64 initrd_image_start = cpu_to_fdt64(initrd_addr);
- status = fdt_setprop(fdt, node, "linux,initrd-start",
- &initrd_image_start, sizeof(u64));
+ status = fdt_setprop_var(fdt, node, "linux,initrd-start", initrd_image_start);
if (status)
goto fdt_set_fail;
+
initrd_image_end = cpu_to_fdt64(initrd_addr + initrd_size);
- status = fdt_setprop(fdt, node, "linux,initrd-end",
- &initrd_image_end, sizeof(u64));
+ status = fdt_setprop_var(fdt, node, "linux,initrd-end", initrd_image_end);
if (status)
goto fdt_set_fail;
}
@@ -117,30 +115,28 @@ static efi_status_t update_fdt(efi_syste
/* Add FDT entries for EFI runtime services in chosen node. */
node = fdt_subnode_offset(fdt, 0, "chosen");
fdt_val64 = cpu_to_fdt64((u64)(unsigned long)sys_table);
- status = fdt_setprop(fdt, node, "linux,uefi-system-table",
- &fdt_val64, sizeof(fdt_val64));
+
+ status = fdt_setprop_var(fdt, node, "linux,uefi-system-table", fdt_val64);
if (status)
goto fdt_set_fail;
fdt_val64 = U64_MAX; /* placeholder */
- status = fdt_setprop(fdt, node, "linux,uefi-mmap-start",
- &fdt_val64, sizeof(fdt_val64));
+
+ status = fdt_setprop_var(fdt, node, "linux,uefi-mmap-start", fdt_val64);
if (status)
goto fdt_set_fail;
fdt_val32 = U32_MAX; /* placeholder */
- status = fdt_setprop(fdt, node, "linux,uefi-mmap-size",
- &fdt_val32, sizeof(fdt_val32));
+
+ status = fdt_setprop_var(fdt, node, "linux,uefi-mmap-size", fdt_val32);
if (status)
goto fdt_set_fail;
- status = fdt_setprop(fdt, node, "linux,uefi-mmap-desc-size",
- &fdt_val32, sizeof(fdt_val32));
+ status = fdt_setprop_var(fdt, node, "linux,uefi-mmap-desc-size", fdt_val32);
if (status)
goto fdt_set_fail;
- status = fdt_setprop(fdt, node, "linux,uefi-mmap-desc-ver",
- &fdt_val32, sizeof(fdt_val32));
+ status = fdt_setprop_var(fdt, node, "linux,uefi-mmap-desc-ver", fdt_val32);
if (status)
goto fdt_set_fail;
@@ -150,8 +146,7 @@ static efi_status_t update_fdt(efi_syste
efi_status = efi_get_random_bytes(sys_table, sizeof(fdt_val64),
(u8 *)&fdt_val64);
if (efi_status == EFI_SUCCESS) {
- status = fdt_setprop(fdt, node, "kaslr-seed",
- &fdt_val64, sizeof(fdt_val64));
+ status = fdt_setprop_var(fdt, node, "kaslr-seed", fdt_val64);
if (status)
goto fdt_set_fail;
} else if (efi_status != EFI_NOT_FOUND) {
@@ -159,7 +154,7 @@ static efi_status_t update_fdt(efi_syste
}
}
- /* shrink the FDT back to its minimum size */
+ /* Shrink the FDT back to its minimum size: */
fdt_pack(fdt);
return EFI_SUCCESS;
@@ -182,26 +177,26 @@ static efi_status_t update_fdt_memmap(vo
return EFI_LOAD_ERROR;
fdt_val64 = cpu_to_fdt64((unsigned long)*map->map);
- err = fdt_setprop_inplace(fdt, node, "linux,uefi-mmap-start",
- &fdt_val64, sizeof(fdt_val64));
+
+ err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-mmap-start", fdt_val64);
if (err)
return EFI_LOAD_ERROR;
fdt_val32 = cpu_to_fdt32(*map->map_size);
- err = fdt_setprop_inplace(fdt, node, "linux,uefi-mmap-size",
- &fdt_val32, sizeof(fdt_val32));
+
+ err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-mmap-size", fdt_val32);
if (err)
return EFI_LOAD_ERROR;
fdt_val32 = cpu_to_fdt32(*map->desc_size);
- err = fdt_setprop_inplace(fdt, node, "linux,uefi-mmap-desc-size",
- &fdt_val32, sizeof(fdt_val32));
+
+ err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-mmap-desc-size", fdt_val32);
if (err)
return EFI_LOAD_ERROR;
fdt_val32 = cpu_to_fdt32(*map->desc_ver);
- err = fdt_setprop_inplace(fdt, node, "linux,uefi-mmap-desc-ver",
- &fdt_val32, sizeof(fdt_val32));
+
+ err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-mmap-desc-ver", fdt_val32);
if (err)
return EFI_LOAD_ERROR;
@@ -209,13 +204,13 @@ static efi_status_t update_fdt_memmap(vo
}
#ifndef EFI_FDT_ALIGN
-#define EFI_FDT_ALIGN EFI_PAGE_SIZE
+# define EFI_FDT_ALIGN EFI_PAGE_SIZE
#endif
struct exit_boot_struct {
- efi_memory_desc_t *runtime_map;
- int *runtime_entry_count;
- void *new_fdt_addr;
+ efi_memory_desc_t *runtime_map;
+ int *runtime_entry_count;
+ void *new_fdt_addr;
};
static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
@@ -235,7 +230,7 @@ static efi_status_t exit_boot_func(efi_s
}
#ifndef MAX_FDT_SIZE
-#define MAX_FDT_SIZE SZ_2M
+# define MAX_FDT_SIZE SZ_2M
#endif
/*
@@ -266,16 +261,16 @@ efi_status_t allocate_new_fdt_and_exit_b
unsigned long mmap_key;
efi_memory_desc_t *memory_map, *runtime_map;
efi_status_t status;
- int runtime_entry_count = 0;
+ int runtime_entry_count;
struct efi_boot_memmap map;
struct exit_boot_struct priv;
- map.map = &runtime_map;
- map.map_size = &map_size;
- map.desc_size = &desc_size;
- map.desc_ver = &desc_ver;
- map.key_ptr = &mmap_key;
- map.buff_size = &buff_size;
+ map.map = &runtime_map;
+ map.map_size = &map_size;
+ map.desc_size = &desc_size;
+ map.desc_ver = &desc_ver;
+ map.key_ptr = &mmap_key;
+ map.buff_size = &buff_size;
/*
* Get a copy of the current memory map that we will use to prepare
@@ -289,15 +284,13 @@ efi_status_t allocate_new_fdt_and_exit_b
return status;
}
- pr_efi(sys_table,
- "Exiting boot services and installing virtual address map...\n");
+ pr_efi(sys_table, "Exiting boot services and installing virtual address map...\n");
map.map = &memory_map;
status = efi_high_alloc(sys_table, MAX_FDT_SIZE, EFI_FDT_ALIGN,
new_fdt_addr, max_addr);
if (status != EFI_SUCCESS) {
- pr_efi_err(sys_table,
- "Unable to allocate memory for new device tree.\n");
+ pr_efi_err(sys_table, "Unable to allocate memory for new device tree.\n");
goto fail;
}
@@ -318,11 +311,12 @@ efi_status_t allocate_new_fdt_and_exit_b
goto fail_free_new_fdt;
}
- priv.runtime_map = runtime_map;
- priv.runtime_entry_count = &runtime_entry_count;
- priv.new_fdt_addr = (void *)*new_fdt_addr;
- status = efi_exit_boot_services(sys_table, handle, &map, &priv,
- exit_boot_func);
+ runtime_entry_count = 0;
+ priv.runtime_map = runtime_map;
+ priv.runtime_entry_count = &runtime_entry_count;
+ priv.new_fdt_addr = (void *)*new_fdt_addr;
+
+ status = efi_exit_boot_services(sys_table, handle, &map, &priv, exit_boot_func);
if (status == EFI_SUCCESS) {
efi_set_virtual_address_map_t *svam;
@@ -363,6 +357,7 @@ fail_free_new_fdt:
fail:
sys_table->boottime->free_pool(runtime_map);
+
return EFI_LOAD_ERROR;
}
@@ -373,7 +368,7 @@ void *get_fdt(efi_system_table_t *sys_ta
void *fdt;
int i;
- tables = (efi_config_table_t *) sys_table->tables;
+ tables = (efi_config_table_t *)sys_table->tables;
fdt = NULL;
for (i = 0; i < sys_table->nr_tables; i++) {
Index: tip/scripts/dtc/libfdt/libfdt.h
===================================================================
--- tip.orig/scripts/dtc/libfdt/libfdt.h
+++ tip/scripts/dtc/libfdt/libfdt.h
@@ -1213,8 +1213,14 @@ int fdt_setprop_inplace_namelen_partial(
* -FDT_ERR_TRUNCATED, standard meanings
*/
#ifndef SWIG /* Not available in Python */
+
int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
const void *val, int len);
+
+/* Helper macro for the usual case of using simple C variables: */
+#define fdt_setprop_inplace_var(fdt, node_offset, name, var) \
+ fdt_setprop_inplace((fdt), (node_offset), (name), &(var), sizeof(var))
+
#endif
/**
@@ -1540,6 +1546,10 @@ int fdt_setprop(void *fdt, int nodeoffse
int fdt_setprop_placeholder(void *fdt, int nodeoffset, const char *name,
int len, void **prop_data);
+/* Helper macro for the usual case of using simple C variables: */
+#define fdt_setprop_var(fdt, node_offset, name, var) \
+ fdt_setprop((fdt), (node_offset), (name), &(var), sizeof(var))
+
/**
* fdt_setprop_u32 - set a property to a 32-bit integer
* @fdt: pointer to the device tree blob
* Ard Biesheuvel <[email protected]> wrote:
> From: YiFei Zhu <[email protected]>
>
> An affected screen resolution is 1366 x 768, which width is not
> divisible by 8, the default font width. On such screens, when longer
> lines are earlyprintk'ed, overflow-to-next-line can never trigger,
> due to the left-most x-coordinate of the next character always less
> than the screen width. Earlyprintk will infinite loop in trying to
> print the rest of the string but unable to, due to the line being
> full.
>
> This patch makes the trigger consider the right-most x-coordinate,
> instead of left-most, as the value to compare against the screen
> width threshold.
>
> Signed-off-by: YiFei Zhu <[email protected]>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> arch/x86/platform/efi/early_printk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/platform/efi/early_printk.c b/arch/x86/platform/efi/early_printk.c
> index 7476b3b097e1..7138bc7a265c 100644
> --- a/arch/x86/platform/efi/early_printk.c
> +++ b/arch/x86/platform/efi/early_printk.c
> @@ -183,7 +183,7 @@ early_efi_write(struct console *con, const char *str, unsigned int num)
> num--;
> }
>
> - if (efi_x >= si->lfb_width) {
> + if (efi_x + font->width > si->lfb_width) {
> efi_x = 0;
> efi_y += font->height;
> }
Any objections to marking this for -stable and queueing it up in
efi/urgent as well?
Thanks,
Ingo
* Ard Biesheuvel <[email protected]> wrote:
> From: Arend van Spriel <[email protected]>
>
> Since commit:
>
> ce2e6db554fa ("brcmfmac: Add support for getting nvram contents from
> EFI variables")
This commit ID is not upstream AFAICS. Which tree is it from? Mentioning
non-upstream sha1's is discouraged in changelogs, as there's no guarantee
that the sha1 will make it upstream.
> we have a device driver accessing the efivars API. Several functions in
> the efivars API assume __efivars is set, i.e., that they will be accessed
> only after efivars_register() has been called. However, the following NULL
> pointer access was reported calling efivar_entry_size() from the brcmfmac
> device driver.
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000008
> pgd = 60bfa5f1
> [00000008] *pgd=00000000
> Internal error: Oops: 5 [#1] SMP ARM
> ...
> Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> Workqueue: events request_firmware_work_func
> PC is at efivar_entry_size+0x28/0x90
> LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
> pc : [<c0c40718>] lr : [<bf2a3ef4>] psr: a00d0113
> sp : ede7fe28 ip : ee983410 fp : c1787f30
> r10: 00000000 r9 : 00000000 r8 : bf2b2258
> r7 : ee983000 r6 : c1604c48 r5 : ede7fe88 r4 : edf337c0
> r3 : 00000000 r2 : 00000000 r1 : ede7fe88 r0 : c17712c8
> Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> Control: 10c5387d Table: ad16804a DAC: 00000051
>
> Disassembly showed that the local static variable __efivars is NULL,
> which is not entirely unexpected given that it is a non-EFI platform.
> So add a NULL pointer check to efivar_entry_size(), and to related
> functions while at it. In efivars_register() a couple of sanity checks
> are added as well.
>
> Cc: Hans de Goede <[email protected]>
> Reported-by: Jon Hunter <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
> Signed-off-by: Ard Biesheuvel <[email protected]>
Will that new commit be backported? If yes I suppose we could mark this
fix -stable too? If not then it's fine for a v4.21 merge.
Thanks,
Ingo
On Fri, 30 Nov 2018 at 08:29, Ingo Molnar <[email protected]> wrote:
>
>
> * Ard Biesheuvel <[email protected]> wrote:
>
> > From: Eric Snowberg <[email protected]>
> >
> > Commit d64934019f6c ("x86/efi: Use efi_exit_boot_services()")
> > introduced a regression on systems with large memory maps
> > causing them to hang on boot. The first "goto get_map" that was removed
> > from exit_boot insured there was enough room for the memory map when
> > efi_call_early(exit_boot_services) was called. This happens when
> > (nr_desc > ARRAY_SIZE(params->e820_table).
> >
> > Chain of events:
> > exit_boot()
> > efi_exit_boot_services()
> > efi_get_memory_map <- at this point the mm can't grow over 8 desc
> > priv_func()
> > exit_boot_func()
> > allocate_e820ext() <- new mm grows over 8 desc from e820 alloc
> > efi_call_early(exit_boot_services) <- mm key doesn't match so retry
> > efi_call_early(get_memory_map) <- not enough room for new mm
> > system hangs
> >
> > This patch allocates the e820 buffer before calling efi_exit_boot_services
> > and fixes the regression.
> >
> > Signed-off-by: Eric Snowberg <[email protected]>
> > Signed-off-by: Ard Biesheuvel <[email protected]>
> > ---
> > arch/x86/boot/compressed/eboot.c | 65 ++++++++++++++++++++------------
> > 1 file changed, 41 insertions(+), 24 deletions(-)
>
> Any objections against marking this for -stable and filing it in
> efi/urgent? Boot hangs are show-stopper bugs, so distributions would want
> to backport this fix anyway.
>
No objections per se, but this is the kind of patch that might go the
other way as well, so I would prefer to give it some wider coverage at
first, given how quickly patches are taken into -stable.
I can make a note of it and send it to Greg halfway into the next -rc cycle.
> > + boot_map.map = ↦
> > + boot_map.map_size = &map_size;
> > + boot_map.desc_size = &desc_size;
> > + boot_map.desc_ver = NULL;
> > + boot_map.key_ptr = NULL;
> > + boot_map.buff_size = &buff_size;
>
> I have changed this to the canonical style, which is also used elsewhere
> in the file:
>
> > map.buff_size = &buff_size;
> > priv.boot_params = boot_params;
> > priv.efi = &boot_params->efi_info;
> > - priv.e820ext = NULL;
> > - priv.e820ext_size = 0;
>
> I.e.:
>
> + boot_map.map = ↦
> + boot_map.map_size = &map_size;
> + boot_map.desc_size = &desc_size;
> + boot_map.desc_ver = NULL;
> + boot_map.key_ptr = NULL;
> + boot_map.buff_size = &buff_size;
> +
> + status = efi_get_memory_map(sys_table, &boot_map);
>
Excellent, thanks.
On Fri, 30 Nov 2018 at 08:57, Ingo Molnar <[email protected]> wrote:
>
>
> * Ard Biesheuvel <[email protected]> wrote:
>
> > From: Julien Thierry <[email protected]>
> >
> > Closing bracket seems to end a for statement when it is actually ending
> > the contained if. Add some brackets to have clear delimitation of each
> > scope.
> >
> > No functional change/fix, just fix the indentation.
> >
> > Signed-off-by: Julien Thierry <[email protected]>
> > Signed-off-by: Ard Biesheuvel <[email protected]>
> > ---
> > drivers/firmware/efi/libstub/fdt.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> > index 0c0d2312f4a8..a3614f9b5f75 100644
> > --- a/drivers/firmware/efi/libstub/fdt.c
> > +++ b/drivers/firmware/efi/libstub/fdt.c
> > @@ -376,7 +376,7 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
> > tables = (efi_config_table_t *) sys_table->tables;
> > fdt = NULL;
> >
> > - for (i = 0; i < sys_table->nr_tables; i++)
> > + for (i = 0; i < sys_table->nr_tables; i++) {
> > if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
> > fdt = (void *) tables[i].table;
> > if (fdt_check_header(fdt) != 0) {
> > @@ -385,7 +385,8 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
> > }
> > *fdt_size = fdt_totalsize(fdt);
> > break;
> > - }
> > + }
> > + }
>
> So if we are doing trivial cleanups, how about the patch below on top? It
> cleans up this file for good. Only minimally build tested.
>
> Thanks,
>
> Ingo
>
> ======================>
> Subject: efi/fdt: More cleanups
> From: Ingo Molnar <[email protected]>
>
> Apply a number of cleanups:
>
> - Introduce fdt_setprop_*var() helper macros to simplify and shorten repetitive
> sequences - this also makes it less likely that the wrong variable size is
> passed in. This change makes a lot of the property-setting calls single-line
> and easier to read.
>
This change looks fine to me, but scripts/dtc/libfdt/libfdt.h is part
of an external library, so we'd either need to contribute it there or
define the macro in a local header.
> - Harmonize comment style: capitalization, punctuation, whitespaces, etc.
>
> - Fix some whitespace noise in the libstub Makefile which I happened to notice.
>
> - Use the standard tabular initialization style:
>
> - map.map = &runtime_map;
> - map.map_size = &map_size;
> - map.desc_size = &desc_size;
> - map.desc_ver = &desc_ver;
> - map.key_ptr = &mmap_key;
> - map.buff_size = &buff_size;
>
> + map.map = &runtime_map;
> + map.map_size = &map_size;
> + map.desc_size = &desc_size;
> + map.desc_ver = &desc_ver;
> + map.key_ptr = &mmap_key;
> + map.buff_size = &buff_size;
>
> - Use tabular structure definition for better readability.
>
> - Make all pr*() lines single-line, even if they marginally exceed 80 cols - this
> makes them visually less intrusive.
>
> - Unbreak line breaks into single lines when the length exceeds 80 cols only
> marginally, for better readability.
>
> - Move assignment closer to the actual usage site.
>
> - Plus some other smaller cleanups, spelling fixes, etc.
>
> No change in functionality intended.
>
> Cc: Linus Torvalds <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Signed-off-by: Ingo Molnar <[email protected]>
Thanks for the cleanup. I will merge it once we decide where to define
the new helper macros.
> ---
> drivers/firmware/efi/libstub/Makefile | 4 -
> drivers/firmware/efi/libstub/fdt.c | 109 ++++++++++++++++------------------
> scripts/dtc/libfdt/libfdt.h | 10 +++
> 3 files changed, 64 insertions(+), 59 deletions(-)
>
> Index: tip/drivers/firmware/efi/libstub/Makefile
> ===================================================================
> --- tip.orig/drivers/firmware/efi/libstub/Makefile
> +++ tip/drivers/firmware/efi/libstub/Makefile
> @@ -49,7 +49,7 @@ lib-$(CONFIG_EFI_ARMSTUB) += arm-stub.o
>
> lib-$(CONFIG_ARM) += arm32-stub.o
> lib-$(CONFIG_ARM64) += arm64-stub.o
> -CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> +CFLAGS_arm64-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
>
> #
> # arm64 puts the stub in the kernel proper, which will unnecessarily retain all
> @@ -86,7 +86,7 @@ quiet_cmd_stubcopy = STUBCPY $@
> cmd_stubcopy = if $(STRIP) --strip-debug $(STUBCOPY_RM-y) -o $@ $<; \
> then if $(OBJDUMP) -r $@ | grep $(STUBCOPY_RELOC-y); \
> then (echo >&2 "$@: absolute symbol references not allowed in the EFI stub"; \
> - rm -f $@; /bin/false); \
> + rm -f $@; /bin/false); \
> else $(OBJCOPY) $(STUBCOPY_FLAGS-y) $< $@; fi \
> else /bin/false; fi
>
> Index: tip/drivers/firmware/efi/libstub/fdt.c
> ===================================================================
> --- tip.orig/drivers/firmware/efi/libstub/fdt.c
> +++ tip/drivers/firmware/efi/libstub/fdt.c
> @@ -26,10 +26,8 @@ static void fdt_update_cell_size(efi_sys
> offset = fdt_path_offset(fdt, "/");
> /* Set the #address-cells and #size-cells values for an empty tree */
>
> - fdt_setprop_u32(fdt, offset, "#address-cells",
> - EFI_DT_ADDR_CELLS_DEFAULT);
> -
> - fdt_setprop_u32(fdt, offset, "#size-cells", EFI_DT_SIZE_CELLS_DEFAULT);
> + fdt_setprop_u32(fdt, offset, "#address-cells", EFI_DT_ADDR_CELLS_DEFAULT);
> + fdt_setprop_u32(fdt, offset, "#size-cells", EFI_DT_SIZE_CELLS_DEFAULT);
> }
>
> static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
> @@ -42,7 +40,7 @@ static efi_status_t update_fdt(efi_syste
> u32 fdt_val32;
> u64 fdt_val64;
>
> - /* Do some checks on provided FDT, if it exists*/
> + /* Do some checks on provided FDT, if it exists: */
> if (orig_fdt) {
> if (fdt_check_header(orig_fdt)) {
> pr_efi_err(sys_table, "Device Tree header not valid!\n");
> @@ -50,7 +48,7 @@ static efi_status_t update_fdt(efi_syste
> }
> /*
> * We don't get the size of the FDT if we get if from a
> - * configuration table.
> + * configuration table:
> */
> if (orig_fdt_size && fdt_totalsize(orig_fdt) > orig_fdt_size) {
> pr_efi_err(sys_table, "Truncated device tree! foo!\n");
> @@ -64,8 +62,8 @@ static efi_status_t update_fdt(efi_syste
> status = fdt_create_empty_tree(fdt, new_fdt_size);
> if (status == 0) {
> /*
> - * Any failure from the following function is non
> - * critical
> + * Any failure from the following function is
> + * non-critical:
> */
> fdt_update_cell_size(sys_table, fdt);
> }
> @@ -86,12 +84,13 @@ static efi_status_t update_fdt(efi_syste
> if (node < 0) {
> node = fdt_add_subnode(fdt, 0, "chosen");
> if (node < 0) {
> - status = node; /* node is error code when negative */
> + /* 'node' is an error code when negative: */
> + status = node;
> goto fdt_set_fail;
> }
> }
>
> - if ((cmdline_ptr != NULL) && (strlen(cmdline_ptr) > 0)) {
> + if (cmdline_ptr != NULL && strlen(cmdline_ptr) > 0) {
> status = fdt_setprop(fdt, node, "bootargs", cmdline_ptr,
> strlen(cmdline_ptr) + 1);
> if (status)
> @@ -103,13 +102,12 @@ static efi_status_t update_fdt(efi_syste
> u64 initrd_image_end;
> u64 initrd_image_start = cpu_to_fdt64(initrd_addr);
>
> - status = fdt_setprop(fdt, node, "linux,initrd-start",
> - &initrd_image_start, sizeof(u64));
> + status = fdt_setprop_var(fdt, node, "linux,initrd-start", initrd_image_start);
> if (status)
> goto fdt_set_fail;
> +
> initrd_image_end = cpu_to_fdt64(initrd_addr + initrd_size);
> - status = fdt_setprop(fdt, node, "linux,initrd-end",
> - &initrd_image_end, sizeof(u64));
> + status = fdt_setprop_var(fdt, node, "linux,initrd-end", initrd_image_end);
> if (status)
> goto fdt_set_fail;
> }
> @@ -117,30 +115,28 @@ static efi_status_t update_fdt(efi_syste
> /* Add FDT entries for EFI runtime services in chosen node. */
> node = fdt_subnode_offset(fdt, 0, "chosen");
> fdt_val64 = cpu_to_fdt64((u64)(unsigned long)sys_table);
> - status = fdt_setprop(fdt, node, "linux,uefi-system-table",
> - &fdt_val64, sizeof(fdt_val64));
> +
> + status = fdt_setprop_var(fdt, node, "linux,uefi-system-table", fdt_val64);
> if (status)
> goto fdt_set_fail;
>
> fdt_val64 = U64_MAX; /* placeholder */
> - status = fdt_setprop(fdt, node, "linux,uefi-mmap-start",
> - &fdt_val64, sizeof(fdt_val64));
> +
> + status = fdt_setprop_var(fdt, node, "linux,uefi-mmap-start", fdt_val64);
> if (status)
> goto fdt_set_fail;
>
> fdt_val32 = U32_MAX; /* placeholder */
> - status = fdt_setprop(fdt, node, "linux,uefi-mmap-size",
> - &fdt_val32, sizeof(fdt_val32));
> +
> + status = fdt_setprop_var(fdt, node, "linux,uefi-mmap-size", fdt_val32);
> if (status)
> goto fdt_set_fail;
>
> - status = fdt_setprop(fdt, node, "linux,uefi-mmap-desc-size",
> - &fdt_val32, sizeof(fdt_val32));
> + status = fdt_setprop_var(fdt, node, "linux,uefi-mmap-desc-size", fdt_val32);
> if (status)
> goto fdt_set_fail;
>
> - status = fdt_setprop(fdt, node, "linux,uefi-mmap-desc-ver",
> - &fdt_val32, sizeof(fdt_val32));
> + status = fdt_setprop_var(fdt, node, "linux,uefi-mmap-desc-ver", fdt_val32);
> if (status)
> goto fdt_set_fail;
>
> @@ -150,8 +146,7 @@ static efi_status_t update_fdt(efi_syste
> efi_status = efi_get_random_bytes(sys_table, sizeof(fdt_val64),
> (u8 *)&fdt_val64);
> if (efi_status == EFI_SUCCESS) {
> - status = fdt_setprop(fdt, node, "kaslr-seed",
> - &fdt_val64, sizeof(fdt_val64));
> + status = fdt_setprop_var(fdt, node, "kaslr-seed", fdt_val64);
> if (status)
> goto fdt_set_fail;
> } else if (efi_status != EFI_NOT_FOUND) {
> @@ -159,7 +154,7 @@ static efi_status_t update_fdt(efi_syste
> }
> }
>
> - /* shrink the FDT back to its minimum size */
> + /* Shrink the FDT back to its minimum size: */
> fdt_pack(fdt);
>
> return EFI_SUCCESS;
> @@ -182,26 +177,26 @@ static efi_status_t update_fdt_memmap(vo
> return EFI_LOAD_ERROR;
>
> fdt_val64 = cpu_to_fdt64((unsigned long)*map->map);
> - err = fdt_setprop_inplace(fdt, node, "linux,uefi-mmap-start",
> - &fdt_val64, sizeof(fdt_val64));
> +
> + err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-mmap-start", fdt_val64);
> if (err)
> return EFI_LOAD_ERROR;
>
> fdt_val32 = cpu_to_fdt32(*map->map_size);
> - err = fdt_setprop_inplace(fdt, node, "linux,uefi-mmap-size",
> - &fdt_val32, sizeof(fdt_val32));
> +
> + err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-mmap-size", fdt_val32);
> if (err)
> return EFI_LOAD_ERROR;
>
> fdt_val32 = cpu_to_fdt32(*map->desc_size);
> - err = fdt_setprop_inplace(fdt, node, "linux,uefi-mmap-desc-size",
> - &fdt_val32, sizeof(fdt_val32));
> +
> + err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-mmap-desc-size", fdt_val32);
> if (err)
> return EFI_LOAD_ERROR;
>
> fdt_val32 = cpu_to_fdt32(*map->desc_ver);
> - err = fdt_setprop_inplace(fdt, node, "linux,uefi-mmap-desc-ver",
> - &fdt_val32, sizeof(fdt_val32));
> +
> + err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-mmap-desc-ver", fdt_val32);
> if (err)
> return EFI_LOAD_ERROR;
>
> @@ -209,13 +204,13 @@ static efi_status_t update_fdt_memmap(vo
> }
>
> #ifndef EFI_FDT_ALIGN
> -#define EFI_FDT_ALIGN EFI_PAGE_SIZE
> +# define EFI_FDT_ALIGN EFI_PAGE_SIZE
> #endif
>
> struct exit_boot_struct {
> - efi_memory_desc_t *runtime_map;
> - int *runtime_entry_count;
> - void *new_fdt_addr;
> + efi_memory_desc_t *runtime_map;
> + int *runtime_entry_count;
> + void *new_fdt_addr;
> };
>
> static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
> @@ -235,7 +230,7 @@ static efi_status_t exit_boot_func(efi_s
> }
>
> #ifndef MAX_FDT_SIZE
> -#define MAX_FDT_SIZE SZ_2M
> +# define MAX_FDT_SIZE SZ_2M
> #endif
>
> /*
> @@ -266,16 +261,16 @@ efi_status_t allocate_new_fdt_and_exit_b
> unsigned long mmap_key;
> efi_memory_desc_t *memory_map, *runtime_map;
> efi_status_t status;
> - int runtime_entry_count = 0;
> + int runtime_entry_count;
> struct efi_boot_memmap map;
> struct exit_boot_struct priv;
>
> - map.map = &runtime_map;
> - map.map_size = &map_size;
> - map.desc_size = &desc_size;
> - map.desc_ver = &desc_ver;
> - map.key_ptr = &mmap_key;
> - map.buff_size = &buff_size;
> + map.map = &runtime_map;
> + map.map_size = &map_size;
> + map.desc_size = &desc_size;
> + map.desc_ver = &desc_ver;
> + map.key_ptr = &mmap_key;
> + map.buff_size = &buff_size;
>
> /*
> * Get a copy of the current memory map that we will use to prepare
> @@ -289,15 +284,13 @@ efi_status_t allocate_new_fdt_and_exit_b
> return status;
> }
>
> - pr_efi(sys_table,
> - "Exiting boot services and installing virtual address map...\n");
> + pr_efi(sys_table, "Exiting boot services and installing virtual address map...\n");
>
> map.map = &memory_map;
> status = efi_high_alloc(sys_table, MAX_FDT_SIZE, EFI_FDT_ALIGN,
> new_fdt_addr, max_addr);
> if (status != EFI_SUCCESS) {
> - pr_efi_err(sys_table,
> - "Unable to allocate memory for new device tree.\n");
> + pr_efi_err(sys_table, "Unable to allocate memory for new device tree.\n");
> goto fail;
> }
>
> @@ -318,11 +311,12 @@ efi_status_t allocate_new_fdt_and_exit_b
> goto fail_free_new_fdt;
> }
>
> - priv.runtime_map = runtime_map;
> - priv.runtime_entry_count = &runtime_entry_count;
> - priv.new_fdt_addr = (void *)*new_fdt_addr;
> - status = efi_exit_boot_services(sys_table, handle, &map, &priv,
> - exit_boot_func);
> + runtime_entry_count = 0;
> + priv.runtime_map = runtime_map;
> + priv.runtime_entry_count = &runtime_entry_count;
> + priv.new_fdt_addr = (void *)*new_fdt_addr;
> +
> + status = efi_exit_boot_services(sys_table, handle, &map, &priv, exit_boot_func);
>
> if (status == EFI_SUCCESS) {
> efi_set_virtual_address_map_t *svam;
> @@ -363,6 +357,7 @@ fail_free_new_fdt:
>
> fail:
> sys_table->boottime->free_pool(runtime_map);
> +
> return EFI_LOAD_ERROR;
> }
>
> @@ -373,7 +368,7 @@ void *get_fdt(efi_system_table_t *sys_ta
> void *fdt;
> int i;
>
> - tables = (efi_config_table_t *) sys_table->tables;
> + tables = (efi_config_table_t *)sys_table->tables;
> fdt = NULL;
>
> for (i = 0; i < sys_table->nr_tables; i++) {
> Index: tip/scripts/dtc/libfdt/libfdt.h
> ===================================================================
> --- tip.orig/scripts/dtc/libfdt/libfdt.h
> +++ tip/scripts/dtc/libfdt/libfdt.h
> @@ -1213,8 +1213,14 @@ int fdt_setprop_inplace_namelen_partial(
> * -FDT_ERR_TRUNCATED, standard meanings
> */
> #ifndef SWIG /* Not available in Python */
> +
> int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
> const void *val, int len);
> +
> +/* Helper macro for the usual case of using simple C variables: */
> +#define fdt_setprop_inplace_var(fdt, node_offset, name, var) \
> + fdt_setprop_inplace((fdt), (node_offset), (name), &(var), sizeof(var))
> +
> #endif
>
> /**
> @@ -1540,6 +1546,10 @@ int fdt_setprop(void *fdt, int nodeoffse
> int fdt_setprop_placeholder(void *fdt, int nodeoffset, const char *name,
> int len, void **prop_data);
>
> +/* Helper macro for the usual case of using simple C variables: */
> +#define fdt_setprop_var(fdt, node_offset, name, var) \
> + fdt_setprop((fdt), (node_offset), (name), &(var), sizeof(var))
> +
> /**
> * fdt_setprop_u32 - set a property to a 32-bit integer
> * @fdt: pointer to the device tree blob
On Fri, 30 Nov 2018 at 09:05, Ingo Molnar <[email protected]> wrote:
>
>
> * Ard Biesheuvel <[email protected]> wrote:
>
> > From: YiFei Zhu <[email protected]>
> >
> > An affected screen resolution is 1366 x 768, which width is not
> > divisible by 8, the default font width. On such screens, when longer
> > lines are earlyprintk'ed, overflow-to-next-line can never trigger,
> > due to the left-most x-coordinate of the next character always less
> > than the screen width. Earlyprintk will infinite loop in trying to
> > print the rest of the string but unable to, due to the line being
> > full.
> >
> > This patch makes the trigger consider the right-most x-coordinate,
> > instead of left-most, as the value to compare against the screen
> > width threshold.
> >
> > Signed-off-by: YiFei Zhu <[email protected]>
> > Signed-off-by: Ard Biesheuvel <[email protected]>
> > ---
> > arch/x86/platform/efi/early_printk.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/platform/efi/early_printk.c b/arch/x86/platform/efi/early_printk.c
> > index 7476b3b097e1..7138bc7a265c 100644
> > --- a/arch/x86/platform/efi/early_printk.c
> > +++ b/arch/x86/platform/efi/early_printk.c
> > @@ -183,7 +183,7 @@ early_efi_write(struct console *con, const char *str, unsigned int num)
> > num--;
> > }
> >
> > - if (efi_x >= si->lfb_width) {
> > + if (efi_x + font->width > si->lfb_width) {
> > efi_x = 0;
> > efi_y += font->height;
> > }
>
> Any objections to marking this for -stable and queueing it up in
> efi/urgent as well?
>
No that is fine.
* Ard Biesheuvel <[email protected]> wrote:
> On Fri, 30 Nov 2018 at 08:29, Ingo Molnar <[email protected]> wrote:
> >
> >
> > * Ard Biesheuvel <[email protected]> wrote:
> >
> > > From: Eric Snowberg <[email protected]>
> > >
> > > Commit d64934019f6c ("x86/efi: Use efi_exit_boot_services()")
> > > introduced a regression on systems with large memory maps
> > > causing them to hang on boot. The first "goto get_map" that was removed
> > > from exit_boot insured there was enough room for the memory map when
> > > efi_call_early(exit_boot_services) was called. This happens when
> > > (nr_desc > ARRAY_SIZE(params->e820_table).
> > >
> > > Chain of events:
> > > exit_boot()
> > > efi_exit_boot_services()
> > > efi_get_memory_map <- at this point the mm can't grow over 8 desc
> > > priv_func()
> > > exit_boot_func()
> > > allocate_e820ext() <- new mm grows over 8 desc from e820 alloc
> > > efi_call_early(exit_boot_services) <- mm key doesn't match so retry
> > > efi_call_early(get_memory_map) <- not enough room for new mm
> > > system hangs
> > >
> > > This patch allocates the e820 buffer before calling efi_exit_boot_services
> > > and fixes the regression.
> > >
> > > Signed-off-by: Eric Snowberg <[email protected]>
> > > Signed-off-by: Ard Biesheuvel <[email protected]>
> > > ---
> > > arch/x86/boot/compressed/eboot.c | 65 ++++++++++++++++++++------------
> > > 1 file changed, 41 insertions(+), 24 deletions(-)
> >
> > Any objections against marking this for -stable and filing it in
> > efi/urgent? Boot hangs are show-stopper bugs, so distributions would want
> > to backport this fix anyway.
> >
>
> No objections per se, but this is the kind of patch that might go the
> other way as well, so I would prefer to give it some wider coverage at
> first, given how quickly patches are taken into -stable.
>
> I can make a note of it and send it to Greg halfway into the next -rc cycle.
So there should be at least one week of testing because I just sent the
EFI fixes to Linus, plus -stable gets at least a week of testing as well.
Also, in practice, -next and early -rc cycles get only the fraction of
testing that later -rc's or -stable gets. So if we want this in -stable
we might as well do it now - or the real testing gets delayed by ~3
months in practice. That's also the pattern encouraged by Linus: if it's
a fix that matters then it should be upstreamed with the usual regression
fixes.
Anyway, your call!
Thanks,
Ingo
On Fri, 30 Nov 2018 at 09:12, Ingo Molnar <[email protected]> wrote:
>
>
> * Ard Biesheuvel <[email protected]> wrote:
>
> > From: Arend van Spriel <[email protected]>
> >
> > Since commit:
> >
> > ce2e6db554fa ("brcmfmac: Add support for getting nvram contents from
> > EFI variables")
>
> This commit ID is not upstream AFAICS. Which tree is it from? Mentioning
> non-upstream sha1's is discouraged in changelogs, as there's no guarantee
> that the sha1 will make it upstream.
>
This is a commit ID from Arend's own tree which is pulled into -next,
so I assumed that he'd only include commit IDs like this if they are
stable.
In any case, the fix itself is rather obvious, so much of the context
provided by the commit log could be summarized as '__efivars may be
NULL so check for that before you dereference it'
> > we have a device driver accessing the efivars API. Several functions in
> > the efivars API assume __efivars is set, i.e., that they will be accessed
> > only after efivars_register() has been called. However, the following NULL
> > pointer access was reported calling efivar_entry_size() from the brcmfmac
> > device driver.
> >
> > Unable to handle kernel NULL pointer dereference at virtual address 00000008
> > pgd = 60bfa5f1
> > [00000008] *pgd=00000000
> > Internal error: Oops: 5 [#1] SMP ARM
> > ...
> > Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
> > Workqueue: events request_firmware_work_func
> > PC is at efivar_entry_size+0x28/0x90
> > LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
> > pc : [<c0c40718>] lr : [<bf2a3ef4>] psr: a00d0113
> > sp : ede7fe28 ip : ee983410 fp : c1787f30
> > r10: 00000000 r9 : 00000000 r8 : bf2b2258
> > r7 : ee983000 r6 : c1604c48 r5 : ede7fe88 r4 : edf337c0
> > r3 : 00000000 r2 : 00000000 r1 : ede7fe88 r0 : c17712c8
> > Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> > Control: 10c5387d Table: ad16804a DAC: 00000051
> >
> > Disassembly showed that the local static variable __efivars is NULL,
> > which is not entirely unexpected given that it is a non-EFI platform.
> > So add a NULL pointer check to efivar_entry_size(), and to related
> > functions while at it. In efivars_register() a couple of sanity checks
> > are added as well.
> >
> > Cc: Hans de Goede <[email protected]>
> > Reported-by: Jon Hunter <[email protected]>
> > Signed-off-by: Arend van Spriel <[email protected]>
> > Signed-off-by: Ard Biesheuvel <[email protected]>
>
> Will that new commit be backported? If yes I suppose we could mark this
> fix -stable too? If not then it's fine for a v4.21 merge.
>
That commit is not -stable material at all, as far as I can tell.
* Ard Biesheuvel <[email protected]> wrote:
> The current implementation of efi_mem_reserve_persistent() is rather
> naive, in the sense that for each invocation, it creates a separate
> linked list entry to describe the reservation. Since the linked list
> entries themselves need to persist across subsequent kexec reboots,
> every reservation created this way results in two memblock_reserve()
> calls at the next boot.
>
> On arm64 systems with 100s of CPUs, this may result in a excessive
> number of memblock reservations, and needless fragmentation.
>
> So instead, make use of the newly updated struct linux_efi_memreserve
> layout to put multiple reservations into a single linked list entry.
> This should get rid of the numerous tiny memblock reservations, and
> effectively cut the total number of reservations in half on arm64
> systems with many CPUs.
>
> Tested-by: Marc Zyngier <[email protected]>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> drivers/firmware/efi/efi.c | 20 +++++++++++++++++---
> include/linux/efi.h | 3 +++
> 2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 80b11521627a..e90bc32c2670 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -998,7 +998,8 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
> {
> struct linux_efi_memreserve *rsv;
> int rsvsize = EFI_MEMRESERVE_SIZE(1);
> - int rc;
> + unsigned long prsv;
> + int rc, index;
>
> if (efi_memreserve_root == (void *)ULONG_MAX)
> return -ENODEV;
> @@ -1009,11 +1010,24 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
> return rc;
> }
>
> - rsv = kmalloc(rsvsize, GFP_ATOMIC);
I fixed the following build warning in this patch:
drivers/firmware/efi/efi.c:1000:6: warning: unused variable ‘rsvsize’ [-Wunused-variable]
'rsvsize' got entirely orphaned by the patch, so it can be removed.
Thanks,
Ingo
On Fri, 30 Nov 2018 at 09:38, Ingo Molnar <[email protected]> wrote:
>
>
> * Ard Biesheuvel <[email protected]> wrote:
>
> > The current implementation of efi_mem_reserve_persistent() is rather
> > naive, in the sense that for each invocation, it creates a separate
> > linked list entry to describe the reservation. Since the linked list
> > entries themselves need to persist across subsequent kexec reboots,
> > every reservation created this way results in two memblock_reserve()
> > calls at the next boot.
> >
> > On arm64 systems with 100s of CPUs, this may result in a excessive
> > number of memblock reservations, and needless fragmentation.
> >
> > So instead, make use of the newly updated struct linux_efi_memreserve
> > layout to put multiple reservations into a single linked list entry.
> > This should get rid of the numerous tiny memblock reservations, and
> > effectively cut the total number of reservations in half on arm64
> > systems with many CPUs.
> >
> > Tested-by: Marc Zyngier <[email protected]>
> > Signed-off-by: Ard Biesheuvel <[email protected]>
> > ---
> > drivers/firmware/efi/efi.c | 20 +++++++++++++++++---
> > include/linux/efi.h | 3 +++
> > 2 files changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 80b11521627a..e90bc32c2670 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -998,7 +998,8 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
> > {
> > struct linux_efi_memreserve *rsv;
> > int rsvsize = EFI_MEMRESERVE_SIZE(1);
> > - int rc;
> > + unsigned long prsv;
> > + int rc, index;
> >
> > if (efi_memreserve_root == (void *)ULONG_MAX)
> > return -ENODEV;
> > @@ -1009,11 +1010,24 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
> > return rc;
> > }
> >
> > - rsv = kmalloc(rsvsize, GFP_ATOMIC);
>
> I fixed the following build warning in this patch:
>
> drivers/firmware/efi/efi.c:1000:6: warning: unused variable ‘rsvsize’ [-Wunused-variable]
>
> 'rsvsize' got entirely orphaned by the patch, so it can be removed.
>
Thanks, that was a rebase error on my part - apologies for not spotting it.
* Ard Biesheuvel <[email protected]> wrote:
> On Fri, 30 Nov 2018 at 08:57, Ingo Molnar <[email protected]> wrote:
> >
> >
> > * Ard Biesheuvel <[email protected]> wrote:
> >
> > > From: Julien Thierry <[email protected]>
> > >
> > > Closing bracket seems to end a for statement when it is actually ending
> > > the contained if. Add some brackets to have clear delimitation of each
> > > scope.
> > >
> > > No functional change/fix, just fix the indentation.
> > >
> > > Signed-off-by: Julien Thierry <[email protected]>
> > > Signed-off-by: Ard Biesheuvel <[email protected]>
> > > ---
> > > drivers/firmware/efi/libstub/fdt.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> > > index 0c0d2312f4a8..a3614f9b5f75 100644
> > > --- a/drivers/firmware/efi/libstub/fdt.c
> > > +++ b/drivers/firmware/efi/libstub/fdt.c
> > > @@ -376,7 +376,7 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
> > > tables = (efi_config_table_t *) sys_table->tables;
> > > fdt = NULL;
> > >
> > > - for (i = 0; i < sys_table->nr_tables; i++)
> > > + for (i = 0; i < sys_table->nr_tables; i++) {
> > > if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
> > > fdt = (void *) tables[i].table;
> > > if (fdt_check_header(fdt) != 0) {
> > > @@ -385,7 +385,8 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
> > > }
> > > *fdt_size = fdt_totalsize(fdt);
> > > break;
> > > - }
> > > + }
> > > + }
> >
> > So if we are doing trivial cleanups, how about the patch below on top? It
> > cleans up this file for good. Only minimally build tested.
> >
> > Thanks,
> >
> > Ingo
> >
> > ======================>
> > Subject: efi/fdt: More cleanups
> > From: Ingo Molnar <[email protected]>
> >
> > Apply a number of cleanups:
> >
> > - Introduce fdt_setprop_*var() helper macros to simplify and shorten repetitive
> > sequences - this also makes it less likely that the wrong variable size is
> > passed in. This change makes a lot of the property-setting calls single-line
> > and easier to read.
> >
>
> This change looks fine to me, but scripts/dtc/libfdt/libfdt.h is part
> of an external library, so we'd either need to contribute it there or
> define the macro in a local header.
Whichever variant you prefer. If the external library has a different
license that requires a separate permission then for that too feel free
to add:
Signed-off-by: Ingo Molnar <[email protected]>
Thanks,
Ingo
Commit-ID: 79c2206d369b87b19ac29cb47601059b6bf5c291
Gitweb: https://git.kernel.org/tip/79c2206d369b87b19ac29cb47601059b6bf5c291
Author: YiFei Zhu <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:30 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 09:05:13 +0100
x86/earlyprintk/efi: Fix infinite loop on some screen widths
An affected screen resolution is 1366 x 768, which width is not
divisible by 8, the default font width. On such screens, when longer
lines are earlyprintk'ed, overflow-to-next-line can never trigger,
due to the left-most x-coordinate of the next character always less
than the screen width. Earlyprintk will infinite loop in trying to
print the rest of the string but unable to, due to the line being
full.
This patch makes the trigger consider the right-most x-coordinate,
instead of left-most, as the value to compare against the screen
width threshold.
Signed-off-by: YiFei Zhu <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Eric Snowberg <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Jon Hunter <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sai Praneeth Prakhya <[email protected]>
Cc: Sedat Dilek <[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/early_printk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/platform/efi/early_printk.c b/arch/x86/platform/efi/early_printk.c
index 7476b3b097e1..7138bc7a265c 100644
--- a/arch/x86/platform/efi/early_printk.c
+++ b/arch/x86/platform/efi/early_printk.c
@@ -183,7 +183,7 @@ early_efi_write(struct console *con, const char *str, unsigned int num)
num--;
}
- if (efi_x >= si->lfb_width) {
+ if (efi_x + font->width > si->lfb_width) {
efi_x = 0;
efi_y += font->height;
}
Commit-ID: b84a64fad40637b1c9fa4f4dbf847a23e29e672b
Gitweb: https://git.kernel.org/tip/b84a64fad40637b1c9fa4f4dbf847a23e29e672b
Author: Eric Snowberg <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:20 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 08:30:07 +0100
x86/efi: Allocate e820 buffer before calling efi_exit_boot_service
The following commit:
d64934019f6c ("x86/efi: Use efi_exit_boot_services()")
introduced a regression on systems with large memory maps causing them
to hang on boot. The first "goto get_map" that was removed from
exit_boot() ensured there was enough room for the memory map when
efi_call_early(exit_boot_services) was called. This happens when
(nr_desc > ARRAY_SIZE(params->e820_table).
Chain of events:
exit_boot()
efi_exit_boot_services()
efi_get_memory_map <- at this point the mm can't grow over 8 desc
priv_func()
exit_boot_func()
allocate_e820ext() <- new mm grows over 8 desc from e820 alloc
efi_call_early(exit_boot_services) <- mm key doesn't match so retry
efi_call_early(get_memory_map) <- not enough room for new mm
system hangs
This patch allocates the e820 buffer before calling efi_exit_boot_services()
and fixes the regression.
[ mingo: minor cleanliness edits. ]
Signed-off-by: Eric Snowberg <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Cc: <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Jon Hunter <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sai Praneeth Prakhya <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: YiFei Zhu <[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 | 65 +++++++++++++++++++++++++---------------
1 file changed, 41 insertions(+), 24 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 8b4c5e001157..544ac4fafd11 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1,3 +1,4 @@
+
/* -----------------------------------------------------------------------
*
* Copyright 2011 Intel Corporation; author Matt Fleming
@@ -634,37 +635,54 @@ static efi_status_t alloc_e820ext(u32 nr_desc, struct setup_data **e820ext,
return status;
}
+static efi_status_t allocate_e820(struct boot_params *params,
+ struct setup_data **e820ext,
+ u32 *e820ext_size)
+{
+ unsigned long map_size, desc_size, buff_size;
+ struct efi_boot_memmap boot_map;
+ efi_memory_desc_t *map;
+ efi_status_t status;
+ __u32 nr_desc;
+
+ boot_map.map = ↦
+ boot_map.map_size = &map_size;
+ boot_map.desc_size = &desc_size;
+ boot_map.desc_ver = NULL;
+ boot_map.key_ptr = NULL;
+ boot_map.buff_size = &buff_size;
+
+ status = efi_get_memory_map(sys_table, &boot_map);
+ if (status != EFI_SUCCESS)
+ return status;
+
+ nr_desc = buff_size / desc_size;
+
+ if (nr_desc > ARRAY_SIZE(params->e820_table)) {
+ u32 nr_e820ext = nr_desc - ARRAY_SIZE(params->e820_table);
+
+ status = alloc_e820ext(nr_e820ext, e820ext, e820ext_size);
+ if (status != EFI_SUCCESS)
+ return status;
+ }
+
+ return EFI_SUCCESS;
+}
+
struct exit_boot_struct {
struct boot_params *boot_params;
struct efi_info *efi;
- struct setup_data *e820ext;
- __u32 e820ext_size;
};
static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
struct efi_boot_memmap *map,
void *priv)
{
- static bool first = true;
const char *signature;
__u32 nr_desc;
efi_status_t status;
struct exit_boot_struct *p = priv;
- if (first) {
- nr_desc = *map->buff_size / *map->desc_size;
- if (nr_desc > ARRAY_SIZE(p->boot_params->e820_table)) {
- u32 nr_e820ext = nr_desc -
- ARRAY_SIZE(p->boot_params->e820_table);
-
- status = alloc_e820ext(nr_e820ext, &p->e820ext,
- &p->e820ext_size);
- if (status != EFI_SUCCESS)
- return status;
- }
- first = false;
- }
-
signature = efi_is_64bit() ? EFI64_LOADER_SIGNATURE
: EFI32_LOADER_SIGNATURE;
memcpy(&p->efi->efi_loader_signature, signature, sizeof(__u32));
@@ -687,8 +705,8 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
{
unsigned long map_sz, key, desc_size, buff_size;
efi_memory_desc_t *mem_map;
- struct setup_data *e820ext;
- __u32 e820ext_size;
+ struct setup_data *e820ext = NULL;
+ __u32 e820ext_size = 0;
efi_status_t status;
__u32 desc_version;
struct efi_boot_memmap map;
@@ -702,8 +720,10 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
map.buff_size = &buff_size;
priv.boot_params = boot_params;
priv.efi = &boot_params->efi_info;
- priv.e820ext = NULL;
- priv.e820ext_size = 0;
+
+ status = allocate_e820(boot_params, &e820ext, &e820ext_size);
+ if (status != EFI_SUCCESS)
+ return status;
/* Might as well exit boot services now */
status = efi_exit_boot_services(sys_table, handle, &map, &priv,
@@ -711,9 +731,6 @@ static efi_status_t exit_boot(struct boot_params *boot_params, void *handle)
if (status != EFI_SUCCESS)
return status;
- e820ext = priv.e820ext;
- e820ext_size = priv.e820ext_size;
-
/* Historic? */
boot_params->alt_mem_k = 32 * 1024;
Commit-ID: ab2180a15ce54739fed381efb4cb12e78dfb1561
Gitweb: https://git.kernel.org/tip/ab2180a15ce54739fed381efb4cb12e78dfb1561
Author: Arend van Spriel <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:27 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 09:06:32 +0100
firmware/efi: Add NULL pointer checks in efivars API functions
Since commit:
ce2e6db554fa ("brcmfmac: Add support for getting nvram contents from EFI variables")
we have a device driver accessing the efivars API. Several functions in
the efivars API assume __efivars is set, i.e., that they will be accessed
only after efivars_register() has been called. However, the following NULL
pointer access was reported calling efivar_entry_size() from the brcmfmac
device driver:
Unable to handle kernel NULL pointer dereference at virtual address 00000008
pgd = 60bfa5f1
[00000008] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
...
Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
Workqueue: events request_firmware_work_func
PC is at efivar_entry_size+0x28/0x90
LR is at brcmf_fw_complete_request+0x3f8/0x8d4 [brcmfmac]
pc : [<c0c40718>] lr : [<bf2a3ef4>] psr: a00d0113
sp : ede7fe28 ip : ee983410 fp : c1787f30
r10: 00000000 r9 : 00000000 r8 : bf2b2258
r7 : ee983000 r6 : c1604c48 r5 : ede7fe88 r4 : edf337c0
r3 : 00000000 r2 : 00000000 r1 : ede7fe88 r0 : c17712c8
Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: ad16804a DAC: 00000051
Disassembly showed that the local static variable __efivars is NULL,
which is not entirely unexpected given that it is a non-EFI platform.
So add a NULL pointer check to efivar_entry_size(), and to related
functions while at it. In efivars_register() a couple of sanity checks
are added as well.
Reported-by: Jon Hunter <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Eric Snowberg <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sai Praneeth Prakhya <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: YiFei Zhu <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
drivers/firmware/efi/vars.c | 99 +++++++++++++++++++++++++++++++++++----------
1 file changed, 78 insertions(+), 21 deletions(-)
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 9336ffdf6e2c..fceaafd67ec6 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -318,7 +318,12 @@ EXPORT_SYMBOL_GPL(efivar_variable_is_removable);
static efi_status_t
check_var_size(u32 attributes, unsigned long size)
{
- const struct efivar_operations *fops = __efivars->ops;
+ const struct efivar_operations *fops;
+
+ if (!__efivars)
+ return EFI_UNSUPPORTED;
+
+ fops = __efivars->ops;
if (!fops->query_variable_store)
return EFI_UNSUPPORTED;
@@ -329,7 +334,12 @@ check_var_size(u32 attributes, unsigned long size)
static efi_status_t
check_var_size_nonblocking(u32 attributes, unsigned long size)
{
- const struct efivar_operations *fops = __efivars->ops;
+ const struct efivar_operations *fops;
+
+ if (!__efivars)
+ return EFI_UNSUPPORTED;
+
+ fops = __efivars->ops;
if (!fops->query_variable_store)
return EFI_UNSUPPORTED;
@@ -429,13 +439,18 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
void *data, bool duplicates, struct list_head *head)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
unsigned long variable_name_size = 1024;
efi_char16_t *variable_name;
efi_status_t status;
efi_guid_t vendor_guid;
int err = 0;
+ if (!__efivars)
+ return -EFAULT;
+
+ ops = __efivars->ops;
+
variable_name = kzalloc(variable_name_size, GFP_KERNEL);
if (!variable_name) {
printk(KERN_ERR "efivars: Memory allocation failed.\n");
@@ -583,12 +598,14 @@ static void efivar_entry_list_del_unlock(struct efivar_entry *entry)
*/
int __efivar_entry_delete(struct efivar_entry *entry)
{
- const struct efivar_operations *ops = __efivars->ops;
efi_status_t status;
- status = ops->set_variable(entry->var.VariableName,
- &entry->var.VendorGuid,
- 0, 0, NULL);
+ if (!__efivars)
+ return -EINVAL;
+
+ status = __efivars->ops->set_variable(entry->var.VariableName,
+ &entry->var.VendorGuid,
+ 0, 0, NULL);
return efi_status_to_err(status);
}
@@ -607,12 +624,17 @@ EXPORT_SYMBOL_GPL(__efivar_entry_delete);
*/
int efivar_entry_delete(struct efivar_entry *entry)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_status_t status;
if (down_interruptible(&efivars_lock))
return -EINTR;
+ if (!__efivars) {
+ up(&efivars_lock);
+ return -EINVAL;
+ }
+ ops = __efivars->ops;
status = ops->set_variable(entry->var.VariableName,
&entry->var.VendorGuid,
0, 0, NULL);
@@ -650,13 +672,19 @@ EXPORT_SYMBOL_GPL(efivar_entry_delete);
int efivar_entry_set(struct efivar_entry *entry, u32 attributes,
unsigned long size, void *data, struct list_head *head)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_status_t status;
efi_char16_t *name = entry->var.VariableName;
efi_guid_t vendor = entry->var.VendorGuid;
if (down_interruptible(&efivars_lock))
return -EINTR;
+
+ if (!__efivars) {
+ up(&efivars_lock);
+ return -EINVAL;
+ }
+ ops = __efivars->ops;
if (head && efivar_entry_find(name, vendor, head, false)) {
up(&efivars_lock);
return -EEXIST;
@@ -687,12 +715,17 @@ static int
efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
u32 attributes, unsigned long size, void *data)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_status_t status;
if (down_trylock(&efivars_lock))
return -EBUSY;
+ if (!__efivars) {
+ up(&efivars_lock);
+ return -EINVAL;
+ }
+
status = check_var_size_nonblocking(attributes,
size + ucs2_strsize(name, 1024));
if (status != EFI_SUCCESS) {
@@ -700,6 +733,7 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
return -ENOSPC;
}
+ ops = __efivars->ops;
status = ops->set_variable_nonblocking(name, &vendor, attributes,
size, data);
@@ -727,9 +761,13 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
bool block, unsigned long size, void *data)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_status_t status;
+ if (!__efivars)
+ return -EINVAL;
+
+ ops = __efivars->ops;
if (!ops->query_variable_store)
return -ENOSYS;
@@ -829,13 +867,18 @@ EXPORT_SYMBOL_GPL(efivar_entry_find);
*/
int efivar_entry_size(struct efivar_entry *entry, unsigned long *size)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_status_t status;
*size = 0;
if (down_interruptible(&efivars_lock))
return -EINTR;
+ if (!__efivars) {
+ up(&efivars_lock);
+ return -EINVAL;
+ }
+ ops = __efivars->ops;
status = ops->get_variable(entry->var.VariableName,
&entry->var.VendorGuid, NULL, size, NULL);
up(&efivars_lock);
@@ -861,12 +904,14 @@ EXPORT_SYMBOL_GPL(efivar_entry_size);
int __efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
unsigned long *size, void *data)
{
- const struct efivar_operations *ops = __efivars->ops;
efi_status_t status;
- status = ops->get_variable(entry->var.VariableName,
- &entry->var.VendorGuid,
- attributes, size, data);
+ if (!__efivars)
+ return -EINVAL;
+
+ status = __efivars->ops->get_variable(entry->var.VariableName,
+ &entry->var.VendorGuid,
+ attributes, size, data);
return efi_status_to_err(status);
}
@@ -882,14 +927,19 @@ EXPORT_SYMBOL_GPL(__efivar_entry_get);
int efivar_entry_get(struct efivar_entry *entry, u32 *attributes,
unsigned long *size, void *data)
{
- const struct efivar_operations *ops = __efivars->ops;
efi_status_t status;
if (down_interruptible(&efivars_lock))
return -EINTR;
- status = ops->get_variable(entry->var.VariableName,
- &entry->var.VendorGuid,
- attributes, size, data);
+
+ if (!__efivars) {
+ up(&efivars_lock);
+ return -EINVAL;
+ }
+
+ status = __efivars->ops->get_variable(entry->var.VariableName,
+ &entry->var.VendorGuid,
+ attributes, size, data);
up(&efivars_lock);
return efi_status_to_err(status);
@@ -921,7 +971,7 @@ EXPORT_SYMBOL_GPL(efivar_entry_get);
int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
unsigned long *size, void *data, bool *set)
{
- const struct efivar_operations *ops = __efivars->ops;
+ const struct efivar_operations *ops;
efi_char16_t *name = entry->var.VariableName;
efi_guid_t *vendor = &entry->var.VendorGuid;
efi_status_t status;
@@ -940,6 +990,11 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
if (down_interruptible(&efivars_lock))
return -EINTR;
+ if (!__efivars) {
+ err = -EINVAL;
+ goto out;
+ }
+
/*
* Ensure that the available space hasn't shrunk below the safe level
*/
@@ -956,6 +1011,8 @@ int efivar_entry_set_get_size(struct efivar_entry *entry, u32 attributes,
}
}
+ ops = __efivars->ops;
+
status = ops->set_variable(name, vendor, attributes, *size, data);
if (status != EFI_SUCCESS) {
err = efi_status_to_err(status);
Commit-ID: 6935b3c43da96bb48017b2a3bc1d4f93899f9b28
Gitweb: https://git.kernel.org/tip/6935b3c43da96bb48017b2a3bc1d4f93899f9b28
Author: Julien Thierry <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:21 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 09:10:29 +0100
efi/fdt: Indentation fix
Closing bracket seems to end a for statement when it is actually ending
the contained if. Add some brackets to have clear delimitation of each
scope.
No functional change/fix, just fix the indentation.
Signed-off-by: Julien Thierry <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Eric Snowberg <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Jon Hunter <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sai Praneeth Prakhya <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: YiFei Zhu <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
drivers/firmware/efi/libstub/fdt.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 0c0d2312f4a8..a3614f9b5f75 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -376,7 +376,7 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
tables = (efi_config_table_t *) sys_table->tables;
fdt = NULL;
- for (i = 0; i < sys_table->nr_tables; i++)
+ for (i = 0; i < sys_table->nr_tables; i++) {
if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
fdt = (void *) tables[i].table;
if (fdt_check_header(fdt) != 0) {
@@ -385,7 +385,8 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
}
*fdt_size = fdt_totalsize(fdt);
break;
- }
+ }
+ }
return fdt;
}
Commit-ID: 7e0dabd3010d6041ee0a952c1146b2150a11f1be
Gitweb: https://git.kernel.org/tip/7e0dabd3010d6041ee0a952c1146b2150a11f1be
Author: Sai Praneeth Prakhya <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:23 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 09:10:30 +0100
x86/mm/pageattr: Introduce helper function to unmap EFI boot services
Ideally, after kernel assumes control of the platform, firmware
shouldn't access EFI boot services code/data regions. But, it's noticed
that this is not so true in many x86 platforms. Hence, during boot,
kernel reserves EFI boot services code/data regions [1] and maps [2]
them to efi_pgd so that call to set_virtual_address_map() doesn't fail.
After returning from set_virtual_address_map(), kernel frees the
reserved regions [3] but they still remain mapped. Hence, introduce
kernel_unmap_pages_in_pgd() which will later be used to unmap EFI boot
services code/data regions.
While at it modify kernel_map_pages_in_pgd() by:
1. Adding __init modifier because it's always used *only* during boot.
2. Add a warning if it's used after SMP is initialized because it uses
__flush_tlb_all() which flushes mappings only on current CPU.
Unmapping EFI boot services code/data regions will result in clearing
PAGE_PRESENT bit and it shouldn't bother L1TF cases because it's already
handled by protnone_mask() at arch/x86/include/asm/pgtable-invert.h.
[1] efi_reserve_boot_services()
[2] efi_map_region() -> __map_region() -> kernel_map_pages_in_pgd()
[3] efi_free_boot_services()
Signed-off-by: Sai Praneeth Prakhya <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Eric Snowberg <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Jon Hunter <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: YiFei Zhu <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/include/asm/pgtable_types.h | 8 ++++++--
arch/x86/mm/pageattr.c | 40 ++++++++++++++++++++++++++++++++++--
2 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 106b7d0e2dae..d6ff0bbdb394 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -564,8 +564,12 @@ extern pte_t *lookup_address_in_pgd(pgd_t *pgd, unsigned long address,
unsigned int *level);
extern pmd_t *lookup_pmd_address(unsigned long address);
extern phys_addr_t slow_virt_to_phys(void *__address);
-extern int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
- unsigned numpages, unsigned long page_flags);
+extern int __init kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn,
+ unsigned long address,
+ unsigned numpages,
+ unsigned long page_flags);
+extern int __init kernel_unmap_pages_in_pgd(pgd_t *pgd, unsigned long address,
+ unsigned long numpages);
#endif /* !__ASSEMBLY__ */
#endif /* _ASM_X86_PGTABLE_DEFS_H */
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index db7a10082238..bac35001d896 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -2338,8 +2338,8 @@ bool kernel_page_present(struct page *page)
#endif /* CONFIG_DEBUG_PAGEALLOC */
-int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
- unsigned numpages, unsigned long page_flags)
+int __init kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
+ unsigned numpages, unsigned long page_flags)
{
int retval = -EINVAL;
@@ -2353,6 +2353,8 @@ int kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
.flags = 0,
};
+ WARN_ONCE(num_online_cpus() > 1, "Don't call after initializing SMP");
+
if (!(__supported_pte_mask & _PAGE_NX))
goto out;
@@ -2374,6 +2376,40 @@ out:
return retval;
}
+/*
+ * __flush_tlb_all() flushes mappings only on current CPU and hence this
+ * function shouldn't be used in an SMP environment. Presently, it's used only
+ * during boot (way before smp_init()) by EFI subsystem and hence is ok.
+ */
+int __init kernel_unmap_pages_in_pgd(pgd_t *pgd, unsigned long address,
+ unsigned long numpages)
+{
+ int retval;
+
+ /*
+ * The typical sequence for unmapping is to find a pte through
+ * lookup_address_in_pgd() (ideally, it should never return NULL because
+ * the address is already mapped) and change it's protections. As pfn is
+ * the *target* of a mapping, it's not useful while unmapping.
+ */
+ struct cpa_data cpa = {
+ .vaddr = &address,
+ .pfn = 0,
+ .pgd = pgd,
+ .numpages = numpages,
+ .mask_set = __pgprot(0),
+ .mask_clr = __pgprot(_PAGE_PRESENT | _PAGE_RW),
+ .flags = 0,
+ };
+
+ WARN_ONCE(num_online_cpus() > 1, "Don't call after initializing SMP");
+
+ retval = __change_page_attr_set_clr(&cpa, 0);
+ __flush_tlb_all();
+
+ return retval;
+}
+
/*
* The testcases use internal knowledge of the implementation that shouldn't
* be exposed to the rest of the kernel. Include these directly here.
Commit-ID: 08cfb38f3ef49cfd1bba11a00401451606477d80
Gitweb: https://git.kernel.org/tip/08cfb38f3ef49cfd1bba11a00401451606477d80
Author: Sai Praneeth Prakhya <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:24 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 09:10:30 +0100
x86/efi: Unmap EFI boot services code/data regions from efi_pgd
efi_free_boot_services(), as the name suggests, frees EFI boot services
code/data regions but forgets to unmap these regions from efi_pgd. This
means that any code that's running in efi_pgd address space (e.g:
any EFI runtime service) would still be able to access these regions but
the contents of these regions would have long been over written by
someone else. So, it's important to unmap these regions. Hence,
introduce efi_unmap_pages() to unmap these regions from efi_pgd.
After unmapping EFI boot services code/data regions, any illegal access
by buggy firmware to these regions would result in page fault which will
be handled by EFI specific fault handler.
Signed-off-by: Sai Praneeth Prakhya <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Acked-by: Thomas Gleixner <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Eric Snowberg <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Jon Hunter <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: YiFei Zhu <[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 | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 95e77a667ba5..09e811b9da26 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -369,6 +369,24 @@ void __init efi_reserve_boot_services(void)
}
}
+/*
+ * Apart from having VA mappings for EFI boot services code/data regions,
+ * (duplicate) 1:1 mappings were also created as a quirk for buggy firmware. So,
+ * unmap both 1:1 and VA mappings.
+ */
+static void __init efi_unmap_pages(efi_memory_desc_t *md)
+{
+ pgd_t *pgd = efi_mm.pgd;
+ u64 pa = md->phys_addr;
+ u64 va = md->virt_addr;
+
+ if (kernel_unmap_pages_in_pgd(pgd, pa, md->num_pages))
+ pr_err("Failed to unmap 1:1 mapping for 0x%llx\n", pa);
+
+ if (kernel_unmap_pages_in_pgd(pgd, va, md->num_pages))
+ pr_err("Failed to unmap VA mapping for 0x%llx\n", va);
+}
+
void __init efi_free_boot_services(void)
{
phys_addr_t new_phys, new_size;
@@ -393,6 +411,13 @@ void __init efi_free_boot_services(void)
continue;
}
+ /*
+ * Before calling set_virtual_address_map(), EFI boot services
+ * code/data regions were mapped as a quirk for buggy firmware.
+ * Unmap them from efi_pgd before freeing them up.
+ */
+ efi_unmap_pages(md);
+
/*
* Nasty quirk: if all sub-1MB memory is used for boot
* services, we can get here without having allocated the
Commit-ID: 8c25db0a5a67986106aa3da7ce165ff961aa7847
Gitweb: https://git.kernel.org/tip/8c25db0a5a67986106aa3da7ce165ff961aa7847
Author: Julien Thierry <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:22 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 09:10:30 +0100
efi/fdt: Simplify the get_fdt() flow
Reorganize the get_fdt() lookup loop, clearly showing that:
- Nothing is done for table entries that do not have fdt_guid
- Once an entry with fdt_guid is found, break out of the loop
No functional changes.
Suggested-by: Joe Perches <[email protected]>
Signed-off-by: Julien Thierry <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Eric Snowberg <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Jon Hunter <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sai Praneeth Prakhya <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: YiFei Zhu <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
drivers/firmware/efi/libstub/fdt.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index a3614f9b5f75..0dc7b4987cc2 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -370,23 +370,24 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size)
{
efi_guid_t fdt_guid = DEVICE_TREE_GUID;
efi_config_table_t *tables;
- void *fdt;
int i;
- tables = (efi_config_table_t *) sys_table->tables;
- fdt = NULL;
+ tables = (efi_config_table_t *)sys_table->tables;
for (i = 0; i < sys_table->nr_tables; i++) {
- if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
- fdt = (void *) tables[i].table;
- if (fdt_check_header(fdt) != 0) {
- pr_efi_err(sys_table, "Invalid header detected on UEFI supplied FDT, ignoring ...\n");
- return NULL;
- }
- *fdt_size = fdt_totalsize(fdt);
- break;
+ void *fdt;
+
+ if (efi_guidcmp(tables[i].guid, fdt_guid) != 0)
+ continue;
+
+ fdt = (void *)tables[i].table;
+ if (fdt_check_header(fdt) != 0) {
+ pr_efi_err(sys_table, "Invalid header detected on UEFI supplied FDT, ignoring ...\n");
+ return NULL;
}
+ *fdt_size = fdt_totalsize(fdt);
+ return fdt;
}
- return fdt;
+ return NULL;
}
Commit-ID: 3db5e0ba8b8f4aee631d7ee04b7a11c56cfdc213
Gitweb: https://git.kernel.org/tip/3db5e0ba8b8f4aee631d7ee04b7a11c56cfdc213
Author: Nathan Chancellor <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:26 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 09:10:31 +0100
efi/libstub: Disable some warnings for x86{,_64}
When building the kernel with Clang, some disabled warnings appear
because this Makefile overrides KBUILD_CFLAGS for x86{,_64}. Add them to
this list so that the build is clean again.
-Wpointer-sign was disabled for the whole kernel before the beginning of Git history.
-Waddress-of-packed-member was disabled for the whole kernel and for
the early boot code in these commits:
bfb38988c51e ("kbuild: clang: Disable 'address-of-packed-member' warning")
20c6c1890455 ("x86/boot: Disable the address-of-packed-member compiler warning").
-Wgnu was disabled for the whole kernel and for the early boot code in
these commits:
61163efae020 ("kbuild: LLVMLinux: Add Kbuild support for building kernel with Clang")
6c3b56b19730 ("x86/boot: Disable Clang warnings about GNU extensions").
[ mingo: Made the changelog more readable. ]
Tested-by: Sedat Dilek <[email protected]>
Signed-off-by: Nathan Chancellor <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Reviewed-by: Sedat Dilek <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Eric Snowberg <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Jon Hunter <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sai Praneeth Prakhya <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: YiFei Zhu <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Link: https://github.com/ClangBuiltLinux/linux/issues/112
Signed-off-by: Ingo Molnar <[email protected]>
---
drivers/firmware/efi/libstub/Makefile | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index c51627660dbb..d9845099635e 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -9,7 +9,10 @@ cflags-$(CONFIG_X86_32) := -march=i386
cflags-$(CONFIG_X86_64) := -mcmodel=small
cflags-$(CONFIG_X86) += -m$(BITS) -D__KERNEL__ -O2 \
-fPIC -fno-strict-aliasing -mno-red-zone \
- -mno-mmx -mno-sse -fshort-wchar
+ -mno-mmx -mno-sse -fshort-wchar \
+ -Wno-pointer-sign \
+ $(call cc-disable-warning, address-of-packed-member) \
+ $(call cc-disable-warning, gnu)
# arm64 uses the full KBUILD_CFLAGS so it's necessary to explicitly
# disable the stackleak plugin
Commit-ID: 47c33a095e1fae376d74b4160a0d73c1a4e73969
Gitweb: https://git.kernel.org/tip/47c33a095e1fae376d74b4160a0d73c1a4e73969
Author: Sai Praneeth Prakhya <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:25 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 09:10:31 +0100
x86/efi: Move efi_<reserve/free>_boot_services() to arch/x86
efi_<reserve/free>_boot_services() are x86 specific quirks and as such
should be in asm/efi.h, so move them from linux/efi.h. Also, call
efi_free_boot_services() from __efi_enter_virtual_mode() as it is x86
specific call and ideally shouldn't be part of init/main.c
Signed-off-by: Sai Praneeth Prakhya <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Acked-by: Thomas Gleixner <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Eric Snowberg <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Jon Hunter <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: YiFei Zhu <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/include/asm/efi.h | 2 ++
arch/x86/platform/efi/efi.c | 2 ++
include/linux/efi.h | 3 ---
init/main.c | 4 ----
4 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index eea40d52ca78..d1e64ac80b9c 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -141,6 +141,8 @@ 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);
extern void efi_recover_from_page_fault(unsigned long phys_addr);
+extern void efi_free_boot_services(void);
+extern void efi_reserve_boot_services(void);
struct efi_setup_data {
u64 fw_vendor;
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 7ae939e353cd..e1cb01a22fa8 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -993,6 +993,8 @@ static void __init __efi_enter_virtual_mode(void)
panic("EFI call to SetVirtualAddressMap() failed!");
}
+ efi_free_boot_services();
+
/*
* Now that EFI is in virtual mode, update the function
* pointers in the runtime service table to the new virtual addresses.
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 100ce4a4aff6..2b3b33c83b05 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1000,13 +1000,11 @@ extern void efi_memmap_walk (efi_freemem_callback_t callback, void *arg);
extern void efi_gettimeofday (struct timespec64 *ts);
extern void efi_enter_virtual_mode (void); /* switch EFI to virtual mode, if possible */
#ifdef CONFIG_X86
-extern void efi_free_boot_services(void);
extern efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size,
bool nonblocking);
extern void efi_find_mirror(void);
#else
-static inline void efi_free_boot_services(void) {}
static inline efi_status_t efi_query_variable_store(u32 attributes,
unsigned long size,
@@ -1046,7 +1044,6 @@ extern void efi_mem_reserve(phys_addr_t addr, u64 size);
extern int efi_mem_reserve_persistent(phys_addr_t addr, u64 size);
extern void efi_initialize_iomem_resources(struct resource *code_resource,
struct resource *data_resource, struct resource *bss_resource);
-extern void efi_reserve_boot_services(void);
extern int efi_get_fdt_params(struct efi_fdt_params *params);
extern struct kobject *efi_kobj;
diff --git a/init/main.c b/init/main.c
index ee147103ba1b..ccefcd8e855f 100644
--- a/init/main.c
+++ b/init/main.c
@@ -737,10 +737,6 @@ asmlinkage __visible void __init start_kernel(void)
arch_post_acpi_subsys_init();
sfi_init_late();
- if (efi_enabled(EFI_RUNTIME_SERVICES)) {
- efi_free_boot_services();
- }
-
/* Do the rest non-__init'ed, we're now alive */
arch_call_rest_init();
}
Commit-ID: 5f0b0ecf043a5319e729c11a53bc8294df12dab3
Gitweb: https://git.kernel.org/tip/5f0b0ecf043a5319e729c11a53bc8294df12dab3
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:28 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 09:10:31 +0100
efi: Permit multiple entries in persistent memreserve data structure
In preparation of updating efi_mem_reserve_persistent() to cause less
fragmentation when dealing with many persistent reservations, update
the struct definition and the code that handles it currently so it
can describe an arbitrary number of reservations using a single linked
list entry. The actual optimization will be implemented in a subsequent
patch.
Tested-by: Marc Zyngier <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Eric Snowberg <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Jon Hunter <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sai Praneeth Prakhya <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: YiFei Zhu <[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 | 39 +++++++++++++++++++++++----------
drivers/firmware/efi/libstub/arm-stub.c | 2 +-
include/linux/efi.h | 13 ++++++++---
3 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 415849bab233..80b11521627a 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -602,21 +602,33 @@ int __init efi_apply_persistent_mem_reservations(void)
while (prsv) {
struct linux_efi_memreserve *rsv;
-
- /* reserve the entry itself */
- memblock_reserve(prsv, sizeof(*rsv));
-
- rsv = early_memremap(prsv, sizeof(*rsv));
- if (rsv == NULL) {
+ u8 *p;
+ int i;
+
+ /*
+ * Just map a full page: that is what we will get
+ * anyway, and it permits us to map the entire entry
+ * before knowing its size.
+ */
+ p = early_memremap(ALIGN_DOWN(prsv, PAGE_SIZE),
+ PAGE_SIZE);
+ if (p == NULL) {
pr_err("Could not map UEFI memreserve entry!\n");
return -ENOMEM;
}
- if (rsv->size)
- memblock_reserve(rsv->base, rsv->size);
+ rsv = (void *)(p + prsv % PAGE_SIZE);
+
+ /* reserve the entry itself */
+ memblock_reserve(prsv, EFI_MEMRESERVE_SIZE(rsv->size));
+
+ for (i = 0; i < atomic_read(&rsv->count); i++) {
+ memblock_reserve(rsv->entry[i].base,
+ rsv->entry[i].size);
+ }
prsv = rsv->next;
- early_memunmap(rsv, sizeof(*rsv));
+ early_memunmap(p, PAGE_SIZE);
}
}
@@ -985,6 +997,7 @@ static int __init efi_memreserve_map_root(void)
int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
{
struct linux_efi_memreserve *rsv;
+ int rsvsize = EFI_MEMRESERVE_SIZE(1);
int rc;
if (efi_memreserve_root == (void *)ULONG_MAX)
@@ -996,12 +1009,14 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
return rc;
}
- rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
+ rsv = kmalloc(rsvsize, GFP_ATOMIC);
if (!rsv)
return -ENOMEM;
- rsv->base = addr;
- rsv->size = size;
+ rsv->size = 1;
+ atomic_set(&rsv->count, 1);
+ rsv->entry[0].base = addr;
+ rsv->entry[0].size = size;
spin_lock(&efi_mem_reserve_persistent_lock);
rsv->next = efi_memreserve_root->next;
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 3d36142cf812..9e20159ea5f5 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -86,8 +86,8 @@ void install_memreserve_table(efi_system_table_t *sys_table_arg)
}
rsv->next = 0;
- rsv->base = 0;
rsv->size = 0;
+ atomic_set(&rsv->count, 0);
status = efi_call_early(install_configuration_table,
&memreserve_table_guid,
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2b3b33c83b05..4f27640fdcdc 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1712,9 +1712,16 @@ extern struct efi_runtime_work efi_rts_work;
extern struct workqueue_struct *efi_rts_wq;
struct linux_efi_memreserve {
- phys_addr_t next;
- phys_addr_t base;
- phys_addr_t size;
+ int size; // allocated size of the array
+ atomic_t count; // number of entries used
+ phys_addr_t next; // pa of next struct instance
+ struct {
+ phys_addr_t base;
+ phys_addr_t size;
+ } entry[0];
};
+#define EFI_MEMRESERVE_SIZE(count) (sizeof(struct linux_efi_memreserve) + \
+ (count) * sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
+
#endif /* _LINUX_EFI_H */
Commit-ID: 80424b02d42bb22f8ff8839cb93a84ade53b39c0
Gitweb: https://git.kernel.org/tip/80424b02d42bb22f8ff8839cb93a84ade53b39c0
Author: Ard Biesheuvel <[email protected]>
AuthorDate: Thu, 29 Nov 2018 18:12:29 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Fri, 30 Nov 2018 09:37:57 +0100
efi: Reduce the amount of memblock reservations for persistent allocations
The current implementation of efi_mem_reserve_persistent() is rather
naive, in the sense that for each invocation, it creates a separate
linked list entry to describe the reservation. Since the linked list
entries themselves need to persist across subsequent kexec reboots,
every reservation created this way results in two memblock_reserve()
calls at the next boot.
On arm64 systems with 100s of CPUs, this may result in a excessive
number of memblock reservations, and needless fragmentation.
So instead, make use of the newly updated struct linux_efi_memreserve
layout to put multiple reservations into a single linked list entry.
This should get rid of the numerous tiny memblock reservations, and
effectively cut the total number of reservations in half on arm64
systems with many CPUs.
[ mingo: build warning fix. ]
Tested-by: Marc Zyngier <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Eric Snowberg <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Jon Hunter <[email protected]>
Cc: Julien Thierry <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Sai Praneeth Prakhya <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: YiFei Zhu <[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 | 21 +++++++++++++++++----
include/linux/efi.h | 3 +++
2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 80b11521627a..4c46ff6f2242 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -997,8 +997,8 @@ static int __init efi_memreserve_map_root(void)
int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
{
struct linux_efi_memreserve *rsv;
- int rsvsize = EFI_MEMRESERVE_SIZE(1);
- int rc;
+ unsigned long prsv;
+ int rc, index;
if (efi_memreserve_root == (void *)ULONG_MAX)
return -ENODEV;
@@ -1009,11 +1009,24 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
return rc;
}
- rsv = kmalloc(rsvsize, GFP_ATOMIC);
+ /* first try to find a slot in an existing linked list entry */
+ for (prsv = efi_memreserve_root->next; prsv; prsv = rsv->next) {
+ rsv = __va(prsv);
+ index = atomic_fetch_add_unless(&rsv->count, 1, rsv->size);
+ if (index < rsv->size) {
+ rsv->entry[index].base = addr;
+ rsv->entry[index].size = size;
+
+ return 0;
+ }
+ }
+
+ /* no slot found - allocate a new linked list entry */
+ rsv = (struct linux_efi_memreserve *)__get_free_page(GFP_ATOMIC);
if (!rsv)
return -ENOMEM;
- rsv->size = 1;
+ rsv->size = EFI_MEMRESERVE_COUNT(PAGE_SIZE);
atomic_set(&rsv->count, 1);
rsv->entry[0].base = addr;
rsv->entry[0].size = size;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 4f27640fdcdc..becd5d76a207 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1724,4 +1724,7 @@ struct linux_efi_memreserve {
#define EFI_MEMRESERVE_SIZE(count) (sizeof(struct linux_efi_memreserve) + \
(count) * sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
+#define EFI_MEMRESERVE_COUNT(size) (((size) - sizeof(struct linux_efi_memreserve)) \
+ / sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
+
#endif /* _LINUX_EFI_H */
On Thu, 29 Nov 2018 at 19:27, Prakhya, Sai Praneeth
<[email protected]> wrote:
>
> Hi Ard,
>
> While building from next branch of efi tree, I noticed the below warning. Could you please check the same on your side?
>
> CC lib/list_debug.o
> drivers/firmware/efi/efi.c: In function 'efi_mem_reserve_persistent':
> drivers/firmware/efi/efi.c:1000:6: warning: unused variable 'rsvsize' [-Wunused-variable]
> int rsvsize = EFI_MEMRESERVE_SIZE(1);
> ^~~~~~~
> CC lib/bitrev.o
> CC net/core/sock_reuseport.o
>
Thanks Sai
Ingo spotted it as well and fixed it up before merging.
> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:[email protected]]
> > Sent: Thursday, November 29, 2018 9:12 AM
> > To: [email protected]; Ingo Molnar <[email protected]>; Thomas
> > Gleixner <[email protected]>
> > Cc: Ard Biesheuvel <[email protected]>; [email protected];
> > Andy Lutomirski <[email protected]>; Arend van Spriel
> > <[email protected]>; Bhupesh Sharma <[email protected]>;
> > Borislav Petkov <[email protected]>; Hansen, Dave <[email protected]>; Eric
> > Snowberg <[email protected]>; Hans de Goede
> > <[email protected]>; Joe Perches <[email protected]>; Jon Hunter
> > <[email protected]>; Julien Thierry <[email protected]>; Marc
> > Zyngier <[email protected]>; Nathan Chancellor
> > <[email protected]>; Peter Zijlstra <[email protected]>; Prakhya,
> > Sai Praneeth <[email protected]>; Sedat Dilek
> > <[email protected]>; YiFei Zhu <[email protected]>
> > Subject: [GIT PULL 00/11] EFI updates
> >
> > The following changes since commit
> > 976b489120cdab2b1b3a41ffa14661db43d58190:
> >
> > efi: Prevent GICv3 WARN() by mapping the memreserve table before first use
> > (2018-11-27 13:50:20 +0100)
> >
> > are available in the Git repository at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-next
> >
> > for you to fetch changes up to 1d29afdbf7ae878a23627ebee81efcd213f11749:
> >
> > efi/x86: earlyprintk - Fix infinite loop on some screen widths (2018-11-28
> > 17:58:42 +0100)
> >
>
> Regards,
> Sai
> On Thu, 29 Nov 2018 at 19:27, Prakhya, Sai Praneeth
> <[email protected]> wrote:
> >
> > Hi Ard,
> >
> > While building from next branch of efi tree, I noticed the below warning. Could
> you please check the same on your side?
> >
> > CC lib/list_debug.o
> > drivers/firmware/efi/efi.c: In function 'efi_mem_reserve_persistent':
> > drivers/firmware/efi/efi.c:1000:6: warning: unused variable 'rsvsize' [-
> Wunused-variable]
> > int rsvsize = EFI_MEMRESERVE_SIZE(1);
> > ^~~~~~~
> > CC lib/bitrev.o
> > CC net/core/sock_reuseport.o
> >
>
> Thanks Sai
>
> Ingo spotted it as well and fixed it up before merging.
Great! :)
Regards,
Sai
> > > > Hi Thomas and Ingo,
> > > >
> > > > I recently noticed that the below commits [1] and [2] are broken
> > > > when kernel command line argument "efi=old_map" is passed. Sorry!
> > > > I missed to test this condition prior to sending these patches to mailing list.
> > > > I am working on a fix and will send it to mailing list as soon as it's ready.
> > > >
> > >
> > > Could you elaborate on the problem please?
> >
> > Sure! My bad..
> >
> > Little bit of history here:
> > Boris with this patch set [1] introduced statically mapping EFI
> > Runtime Services at -4G and also introduced "efi=old_map" to return to
> > previous EFI functionality which used ioremap and __va(pa).
> >
> > [3] and [4] are links to old_map_region()
> >
> > The commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data
> > regions from efi_pgd"), unmaps EFI boot services code/data regions
> > *only* from efi_pgd but efi=old_map maps EFI boot services code/data
> > regions into swapper_pgd. Also, efi=old_map uses either
> > ioremap() or __va(md->phys_addr) to map EFI runtime/boot time services and
> doesn't use kernel_map_pages_in_pgd().
> >
> > So, we need a different unmapping routine to unmap EFI boot services
> > code/data regions from swapper_pgd if they were mapped using efi=old_map.
> >
>
> For the short term, could we simply make your changes dependent on efi !=
> old_map? That gives us some time to fix the old_map case properly.
Yes, I think we could and it should work but I hesitated to propose it because
(as you already noted) it's a short term fix and not the right fix.
Alternatively, we could also evaluate if we need to support efi=old_map case going further.
I thought dropping it would be a bad idea because it changes kernel user visible interface
(because it's a kernel command line argument) and never brought it up.
Not sure what Thomas, Ingo or Linus might think about dropping a kernel command line
argument.
Regards,
Sai
> Commit-ID: 08cfb38f3ef49cfd1bba11a00401451606477d80
> Gitweb:
> https://git.kernel.org/tip/08cfb38f3ef49cfd1bba11a00401451606477d80
> Author: Sai Praneeth Prakhya <[email protected]>
> AuthorDate: Thu, 29 Nov 2018 18:12:24 +0100
> Committer: Ingo Molnar <[email protected]>
> CommitDate: Fri, 30 Nov 2018 09:10:30 +0100
>
> x86/efi: Unmap EFI boot services code/data regions from efi_pgd
>
> efi_free_boot_services(), as the name suggests, frees EFI boot services
> code/data regions but forgets to unmap these regions from efi_pgd. This means
> that any code that's running in efi_pgd address space (e.g:
> any EFI runtime service) would still be able to access these regions but the
> contents of these regions would have long been over written by someone else.
> So, it's important to unmap these regions. Hence, introduce efi_unmap_pages()
> to unmap these regions from efi_pgd.
>
> After unmapping EFI boot services code/data regions, any illegal access by
> buggy firmware to these regions would result in page fault which will be handled
> by EFI specific fault handler.
Hi Thomas and Ingo,
I recently noticed that the below commits [1] and [2] are broken when kernel command line
argument "efi=old_map" is passed. Sorry! I missed to test this condition prior to sending
these patches to mailing list. I am working on a fix and will send it to mailing list as
soon as it's ready.
Meanwhile, could you please drop these patches before sending pull request to Linus?
[1] Commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd")
[2] Commit 7e0dabd3010d ("x86/mm/pageattr: Introduce helper function to unmap EFI boot services")
Regards,
Sai
On Mon, 17 Dec 2018 at 19:06, Prakhya, Sai Praneeth
<[email protected]> wrote:
>
> > Commit-ID: 08cfb38f3ef49cfd1bba11a00401451606477d80
> > Gitweb:
> > https://git.kernel.org/tip/08cfb38f3ef49cfd1bba11a00401451606477d80
> > Author: Sai Praneeth Prakhya <[email protected]>
> > AuthorDate: Thu, 29 Nov 2018 18:12:24 +0100
> > Committer: Ingo Molnar <[email protected]>
> > CommitDate: Fri, 30 Nov 2018 09:10:30 +0100
> >
> > x86/efi: Unmap EFI boot services code/data regions from efi_pgd
> >
> > efi_free_boot_services(), as the name suggests, frees EFI boot services
> > code/data regions but forgets to unmap these regions from efi_pgd. This means
> > that any code that's running in efi_pgd address space (e.g:
> > any EFI runtime service) would still be able to access these regions but the
> > contents of these regions would have long been over written by someone else.
> > So, it's important to unmap these regions. Hence, introduce efi_unmap_pages()
> > to unmap these regions from efi_pgd.
> >
> > After unmapping EFI boot services code/data regions, any illegal access by
> > buggy firmware to these regions would result in page fault which will be handled
> > by EFI specific fault handler.
>
> Hi Thomas and Ingo,
>
> I recently noticed that the below commits [1] and [2] are broken when kernel command line
> argument "efi=old_map" is passed. Sorry! I missed to test this condition prior to sending
> these patches to mailing list. I am working on a fix and will send it to mailing list as
> soon as it's ready.
>
Could you elaborate on the problem please?
> Meanwhile, could you please drop these patches before sending pull request to Linus?
>
> [1] Commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd")
> [2] Commit 7e0dabd3010d ("x86/mm/pageattr: Introduce helper function to unmap EFI boot services")
>
I'd like to understand what the issue is before we drop anything.
> > Hi Thomas and Ingo,
> >
> > I recently noticed that the below commits [1] and [2] are broken when
> > kernel command line argument "efi=old_map" is passed. Sorry! I missed
> > to test this condition prior to sending these patches to mailing list.
> > I am working on a fix and will send it to mailing list as soon as it's ready.
> >
>
> Could you elaborate on the problem please?
Sure! My bad..
Little bit of history here:
Boris with this patch set [1] introduced statically mapping EFI Runtime Services at -4G
and also introduced "efi=old_map" to return to previous EFI functionality which used
ioremap and __va(pa).
[3] and [4] are links to old_map_region()
The commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd"),
unmaps EFI boot services code/data regions *only* from efi_pgd but efi=old_map maps
EFI boot services code/data regions into swapper_pgd. Also, efi=old_map uses either
ioremap() or __va(md->phys_addr) to map EFI runtime/boot time services and doesn't use kernel_map_pages_in_pgd().
So, we need a different unmapping routine to unmap EFI boot services code/data
regions from swapper_pgd if they were mapped using efi=old_map.
[1] https://lkml.org/lkml/2013/9/19/235 - Cover letter for EFI runtime services virtual mapping
[2] https://lkml.org/lkml/2013/10/8/777 - Talks about efi=old_map
[3] https://elixir.bootlin.com/linux/v4.20-rc7/source/arch/x86/platform/efi/efi_64.c#L429
[4] https://elixir.bootlin.com/linux/v4.20-rc7/source/arch/x86/platform/efi/efi.c#L584
>
> > Meanwhile, could you please drop these patches before sending pull request
> to Linus?
> >
> > [1] Commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data
> > regions from efi_pgd") [2] Commit 7e0dabd3010d ("x86/mm/pageattr:
> > Introduce helper function to unmap EFI boot services")
> >
>
> I'd like to understand what the issue is before we drop anything.
Regards,
Sai
On Mon, 17 Dec 2018 at 19:42, Prakhya, Sai Praneeth
<[email protected]> wrote:
>
> > > Hi Thomas and Ingo,
> > >
> > > I recently noticed that the below commits [1] and [2] are broken when
> > > kernel command line argument "efi=old_map" is passed. Sorry! I missed
> > > to test this condition prior to sending these patches to mailing list.
> > > I am working on a fix and will send it to mailing list as soon as it's ready.
> > >
> >
> > Could you elaborate on the problem please?
>
> Sure! My bad..
>
> Little bit of history here:
> Boris with this patch set [1] introduced statically mapping EFI Runtime Services at -4G
> and also introduced "efi=old_map" to return to previous EFI functionality which used
> ioremap and __va(pa).
>
> [3] and [4] are links to old_map_region()
>
> The commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd"),
> unmaps EFI boot services code/data regions *only* from efi_pgd but efi=old_map maps
> EFI boot services code/data regions into swapper_pgd. Also, efi=old_map uses either
> ioremap() or __va(md->phys_addr) to map EFI runtime/boot time services and doesn't use kernel_map_pages_in_pgd().
>
> So, we need a different unmapping routine to unmap EFI boot services code/data
> regions from swapper_pgd if they were mapped using efi=old_map.
>
For the short term, could we simply make your changes dependent on efi
!= old_map? That gives us some time to fix the old_map case properly.
On Fri, 21 Dec 2018 at 18:13, Borislav Petkov <[email protected]> wrote:
>
> On Fri, Dec 21, 2018 at 06:02:29PM +0100, Ard Biesheuvel wrote:
> > As far as I can tell, the SGI x86 UV platforms still rely on this, so
> > we're stuck with it for the foreseeable future.
>
> What happened with the old apple laptops which couldn't handle high
> virtual mappings and needed 1:1? We don't care anymore?
>
If that is the case (I wouldn't know) then yes, there is a second
reason why we need to keep this code.
> > > For the short term, could we simply make your changes dependent on
> > > efi != old_map? That gives us some time to fix the old_map case properly.
> >
> > Yes, I think we could and it should work but I hesitated to propose it
> > because (as you already noted) it's a short term fix and not the right fix.
> >
>
> What is the status here?
Making the unmapping code conditional on !old_map is ready and I will send it out.
I am working on unmapping boot services code/data when old_map is enabled
and ran into issues with memblock and direct mapping in kernel. Will post those details
in a separate thread.
>
> > Alternatively, we could also evaluate if we need to support efi=old_map case
> going further.
> > I thought dropping it would be a bad idea because it changes kernel
> > user visible interface (because it's a kernel command line argument) and never
> brought it up.
> > Not sure what Thomas, Ingo or Linus might think about dropping a
> > kernel command line argument.
> >
>
> Dropping a command line argument is not a problem in itself, unless anyone is
> actively using it :-)
>
> As far as I can tell, the SGI x86 UV platforms still rely on this, so we're stuck with
> it for the foreseeable future.
Thanks (also Boris) for the info. Makes sense why we need efi=old_map.
>
> This means we need a fixes that makes your unmapping code conditional on
> !old_memmap. Do you have an ETA for that?
Sure! I will do some more testing and if it works as expected, will send it before this Sunday.
Regards,
Sai
On Fri, Dec 21, 2018 at 06:02:29PM +0100, Ard Biesheuvel wrote:
> As far as I can tell, the SGI x86 UV platforms still rely on this, so
> we're stuck with it for the foreseeable future.
What happened with the old apple laptops which couldn't handle high
virtual mappings and needed 1:1? We don't care anymore?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Mon, 17 Dec 2018 at 20:48, Prakhya, Sai Praneeth
<[email protected]> wrote:
>
> > > > > Hi Thomas and Ingo,
> > > > >
> > > > > I recently noticed that the below commits [1] and [2] are broken
> > > > > when kernel command line argument "efi=old_map" is passed. Sorry!
> > > > > I missed to test this condition prior to sending these patches to mailing list.
> > > > > I am working on a fix and will send it to mailing list as soon as it's ready.
> > > > >
> > > >
> > > > Could you elaborate on the problem please?
> > >
> > > Sure! My bad..
> > >
> > > Little bit of history here:
> > > Boris with this patch set [1] introduced statically mapping EFI
> > > Runtime Services at -4G and also introduced "efi=old_map" to return to
> > > previous EFI functionality which used ioremap and __va(pa).
> > >
> > > [3] and [4] are links to old_map_region()
> > >
> > > The commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data
> > > regions from efi_pgd"), unmaps EFI boot services code/data regions
> > > *only* from efi_pgd but efi=old_map maps EFI boot services code/data
> > > regions into swapper_pgd. Also, efi=old_map uses either
> > > ioremap() or __va(md->phys_addr) to map EFI runtime/boot time services and
> > doesn't use kernel_map_pages_in_pgd().
> > >
> > > So, we need a different unmapping routine to unmap EFI boot services
> > > code/data regions from swapper_pgd if they were mapped using efi=old_map.
> > >
> >
> > For the short term, could we simply make your changes dependent on efi !=
> > old_map? That gives us some time to fix the old_map case properly.
>
> Yes, I think we could and it should work but I hesitated to propose it because
> (as you already noted) it's a short term fix and not the right fix.
>
What is the status here?
> Alternatively, we could also evaluate if we need to support efi=old_map case going further.
> I thought dropping it would be a bad idea because it changes kernel user visible interface
> (because it's a kernel command line argument) and never brought it up.
> Not sure what Thomas, Ingo or Linus might think about dropping a kernel command line
> argument.
>
Dropping a command line argument is not a problem in itself, unless
anyone is actively using it :-)
As far as I can tell, the SGI x86 UV platforms still rely on this, so
we're stuck with it for the foreseeable future.
This means we need a fixes that makes your unmapping code conditional
on !old_memmap. Do you have an ETA for that?
On Fri, Dec 21, 2018 at 06:26:23PM +0100, Ard Biesheuvel wrote:
> On Fri, 21 Dec 2018 at 18:13, Borislav Petkov <[email protected]> wrote:
> >
> > On Fri, Dec 21, 2018 at 06:02:29PM +0100, Ard Biesheuvel wrote:
> > > As far as I can tell, the SGI x86 UV platforms still rely on this, so
> > > we're stuck with it for the foreseeable future.
> >
> > What happened with the old apple laptops which couldn't handle high
> > virtual mappings and needed 1:1? We don't care anymore?
> >
>
> If that is the case (I wouldn't know) then yes, there is a second
> reason why we need to keep this code.
Fleming knows details and he's on CC, lemme "pull" him up into To: :-)
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Fri, 21 Dec 2018 at 20:29, Borislav Petkov <[email protected]> wrote:
>
> On Fri, Dec 21, 2018 at 06:26:23PM +0100, Ard Biesheuvel wrote:
> > On Fri, 21 Dec 2018 at 18:13, Borislav Petkov <[email protected]> wrote:
> > >
> > > On Fri, Dec 21, 2018 at 06:02:29PM +0100, Ard Biesheuvel wrote:
> > > > As far as I can tell, the SGI x86 UV platforms still rely on this, so
> > > > we're stuck with it for the foreseeable future.
> > >
> > > What happened with the old apple laptops which couldn't handle high
> > > virtual mappings and needed 1:1? We don't care anymore?
> > >
> >
> > If that is the case (I wouldn't know) then yes, there is a second
> > reason why we need to keep this code.
>
> Fleming knows details and he's on CC, lemme "pull" him up into To: :-)
>
IIUC the 1:1 mapping and the 'old' mapping are two different things,
and the new mapping also contains a 1:1 mapping of the boot services
regions, at least until SetVirtualAddressMap() returns.
On Sat, 22 Dec, at 12:07:48PM, Ard Biesheuvel wrote:
> On Fri, 21 Dec 2018 at 20:29, Borislav Petkov <[email protected]> wrote:
> >
> > On Fri, Dec 21, 2018 at 06:26:23PM +0100, Ard Biesheuvel wrote:
> > > On Fri, 21 Dec 2018 at 18:13, Borislav Petkov <[email protected]> wrote:
> > > >
> > > > On Fri, Dec 21, 2018 at 06:02:29PM +0100, Ard Biesheuvel wrote:
> > > > > As far as I can tell, the SGI x86 UV platforms still rely on this, so
> > > > > we're stuck with it for the foreseeable future.
> > > >
> > > > What happened with the old apple laptops which couldn't handle high
> > > > virtual mappings and needed 1:1? We don't care anymore?
> > > >
> > >
> > > If that is the case (I wouldn't know) then yes, there is a second
> > > reason why we need to keep this code.
> >
> > Fleming knows details and he's on CC, lemme "pull" him up into To: :-)
> >
>
> IIUC the 1:1 mapping and the 'old' mapping are two different things,
> and the new mapping also contains a 1:1 mapping of the boot services
> regions, at least until SetVirtualAddressMap() returns.
Yep, they're different. And yes the 1:1 mapping should stick around
with the new scheme IIRC (it's been a while).