efi: map memreserve table before first use

PATCH New Ard Biesheuvel 4 participants 6 messages Nov 20, 2018 – Nov 21, 2018 EFI

Share & tools

Ard Biesheuvel

author

2018-11-20 17:35 UTC #0 lore raw

PATCH 1/1 efi: map memreserve table before first use

Mapping the MEMRESERVE EFI configuration table from an early initcall is too late: the GICv3 ITS code that creates persistent reservations for the boot CPU's LPI tables is invoked from init_IRQ(), which runs much earlier than the handling of the initcalls.

So instead, move the initialization performed by the initcall into efi_mem_reserve_persistent() itself.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/efi.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

drivers/firmware/efi/efi.c +10 ~~-16~~

index fad7c62cfc0e..40de2f6734cc 100644
@@ -967,15 +967,23 @@ bool efi_is_table_address(unsigned long phys_addr)
 }
 
 static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
-static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
+static struct linux_efi_memreserve *efi_memreserve_root;
 
 int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 {
 	struct linux_efi_memreserve *rsv;
 
-	if (!efi_memreserve_root)
+	if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
 		return -ENODEV;
 
+	if (!efi_memreserve_root) {
+		efi_memreserve_root = memremap(efi.mem_reserve,
+					       sizeof(*efi_memreserve_root),
+					       MEMREMAP_WB);
+		if (WARN_ON_ONCE(!efi_memreserve_root))
+			return -ENOMEM;
+	}
+
 	rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
 	if (!rsv)
 		return -ENOMEM;

@@ -991,20 +999,6 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 	return 0;
 }
 
-static int __init efi_memreserve_root_init(void)
-{
-	if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
-		return -ENODEV;
-
-	efi_memreserve_root = memremap(efi.mem_reserve,
-				       sizeof(*efi_memreserve_root),
-				       MEMREMAP_WB);
-	if (!efi_memreserve_root)
-		return -ENOMEM;
-	return 0;
-}
-early_initcall(efi_memreserve_root_init);
-
 #ifdef CONFIG_KEXEC
 static int update_efi_random_seed(struct notifier_block *nb,
 				  unsigned long code, void *unused)

Signature · 1 line

2.17.1

Marc Zyngier

2018-11-20 18:29 UTC #1 lore raw

PATCH 1/1 efi: map memreserve table before first use

Hi Ard,

Quoted 9 lines · Ard Biesheuvel

Mapping the MEMRESERVE EFI configuration table from an early initcall is too late: the GICv3 ITS code that creates persistent reservations for the boot CPU's LPI tables is invoked from init_IRQ(), which runs much earlier than the handling of the initcalls.
So instead, move the initialization performed by the initcall into efi_mem_reserve_persistent() itself.
Signed-off-by Ard Biesheuvel <[email protected]>

I've just given it a go on one of my TX2s, and it boots just fine. So for that:

Tested-by Marc Zyngier <[email protected]>

A comment below though:

Quoted 19 lines

---
 drivers/firmware/efi/efi.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

drivers/firmware/efi/efi.c +1 -1

index fad7c62cfc0e..40de2f6734cc 100644
@@ -967,15 +967,23 @@ bool efi_is_table_address(unsigned long phys_addr)
 }
 
 static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
-static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
+static struct linux_efi_memreserve *efi_memreserve_root;
 
 int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 {
 	struct linux_efi_memreserve *rsv;

drivers/firmware/efi/efi.c bool efi_is_table_address(unsigned long phys_addr) · line 984

Quoted 10 lines

drivers/firmware/efi/efi.c +7 -1

-	if (!efi_memreserve_root)
+	if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
 		return -ENODEV;
 
+	if (!efi_memreserve_root) {
+		efi_memreserve_root = memremap(efi.mem_reserve,
+					       sizeof(*efi_memreserve_root),
+					       MEMREMAP_WB);
+		if (WARN_ON_ONCE(!efi_memreserve_root))
+			return -ENOMEM;

This is now a bit racy if there is more than a single online CPU.

Quoted 12 lines

drivers/firmware/efi/efi.c +2 -3

+	}
+
 	rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
 	if (!rsv)
 		return -ENOMEM;

@@ -991,20 +999,6 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 	return 0;
 }
 
-static int __init efi_memreserve_root_init(void)
-{
-	if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)

drivers/firmware/efi/efi.c int efi_mem_reserve_persistent(phys_addr_t addr, u64 size) · line 1006

Quoted 10 lines

drivers/firmware/efi/efi.c +0 ~~-10~~

-		return -ENODEV;
-
-	efi_memreserve_root = memremap(efi.mem_reserve,
-				       sizeof(*efi_memreserve_root),
-				       MEMREMAP_WB);
-	if (!efi_memreserve_root)
-		return -ENOMEM;
-	return 0;
-}
-early_initcall(efi_memreserve_root_init);

But if we keep this (+ a check that the root is indeed NULL), we should be able to make sure efi_memreserve_root is set before we enable a secondary CPU. Still fragile though.

Thoughts?

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Ard Biesheuvel

author

2018-11-20 18:35 UTC #2 lore raw

PATCH 1/1 efi: map memreserve table before first use

Quoted 71 lines · Marc Zyngier

Hi Ard,

Quoted 9 lines · Ard Biesheuvel

Mapping the MEMRESERVE EFI configuration table from an early initcall is too late: the GICv3 ITS code that creates persistent reservations for the boot CPU's LPI tables is invoked from init_IRQ(), which runs much earlier than the handling of the initcalls.
So instead, move the initialization performed by the initcall into efi_mem_reserve_persistent() itself.
Signed-off-by Ard Biesheuvel <[email protected]>

I've just given it a go on one of my TX2s, and it boots just fine. So for that:

Tested-by Marc Zyngier <[email protected]>

A comment below though:

Quoted 19 lines

---
 drivers/firmware/efi/efi.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

drivers/firmware/efi/efi.c +1 -1

index fad7c62cfc0e..40de2f6734cc 100644
@@ -967,15 +967,23 @@ bool efi_is_table_address(unsigned long phys_addr)
 }
 
 static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
-static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
+static struct linux_efi_memreserve *efi_memreserve_root;
 
 int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 {
      struct linux_efi_memreserve *rsv;

drivers/firmware/efi/efi.c bool efi_is_table_address(unsigned long phys_addr) · line 984

Quoted 10 lines

drivers/firmware/efi/efi.c +7 -1

-     if (!efi_memreserve_root)
+     if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
              return -ENODEV;
 
+     if (!efi_memreserve_root) {
+             efi_memreserve_root = memremap(efi.mem_reserve,
+                                            sizeof(*efi_memreserve_root),
+                                            MEMREMAP_WB);
+             if (WARN_ON_ONCE(!efi_memreserve_root))
+                     return -ENOMEM;

This is now a bit racy if there is more than a single online CPU.

Quoted 18 lines

drivers/firmware/efi/efi.c +2 -9

+     }
+
      rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
      if (!rsv)
              return -ENOMEM;

@@ -991,20 +999,6 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
      return 0;
 }
 
-static int __init efi_memreserve_root_init(void)
-{
-     if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
-             return -ENODEV;
-
-     efi_memreserve_root = memremap(efi.mem_reserve,
-                                    sizeof(*efi_memreserve_root),
-                                    MEMREMAP_WB);
-     if (!efi_memreserve_root)

Quoted 9 lines

drivers/firmware/efi/efi.c int efi_mem_reserve_persistent(phys_addr_t addr, u64 size) · line 1006
Quoted 4 lines
drivers/firmware/efi/efi.c +0 -4
-             return -ENOMEM;
-     return 0;
-}
-early_initcall(efi_memreserve_root_init);
But if we keep this (+ a check that the root is indeed NULL), we should be able to make sure efi_memreserve_root is set before we enable a secondary CPU. Still fragile though.

Why is that stil fragile? It sure isn't pretty, but early initcalls run before SMP boot so it should always work as expected no?

Marc Zyngier

2018-11-20 18:44 UTC #3 lore raw

PATCH 1/1 efi: map memreserve table before first use

Quoted 82 lines · Ard Biesheuvel

Quoted 71 lines · Marc Zyngier
Hi Ard,
Quoted 9 lines · Ard Biesheuvel
Mapping the MEMRESERVE EFI configuration table from an early initcall is too late: the GICv3 ITS code that creates persistent reservations for the boot CPU's LPI tables is invoked from init_IRQ(), which runs much earlier than the handling of the initcalls.
So instead, move the initialization performed by the initcall into efi_mem_reserve_persistent() itself.
Signed-off-by Ard Biesheuvel <[email protected]>
I've just given it a go on one of my TX2s, and it boots just fine. So for that:
Tested-by Marc Zyngier <[email protected]>
A comment below though:
Quoted 19 lines
---
 drivers/firmware/efi/efi.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)
drivers/firmware/efi/efi.c +1 -1
index fad7c62cfc0e..40de2f6734cc 100644
@@ -967,15 +967,23 @@ bool efi_is_table_address(unsigned long phys_addr)
 }
 
 static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
-static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
+static struct linux_efi_memreserve *efi_memreserve_root;
 
 int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 {
      struct linux_efi_memreserve *rsv;
 
drivers/firmware/efi/efi.c bool efi_is_table_address(unsigned long phys_addr) · line 984
Quoted 10 lines
drivers/firmware/efi/efi.c +7 -1
-     if (!efi_memreserve_root)
+     if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
              return -ENODEV;
 
+     if (!efi_memreserve_root) {
+             efi_memreserve_root = memremap(efi.mem_reserve,
+                                            sizeof(*efi_memreserve_root),
+                                            MEMREMAP_WB);
+             if (WARN_ON_ONCE(!efi_memreserve_root))
+                     return -ENOMEM;
This is now a bit racy if there is more than a single online CPU.
Quoted 18 lines
drivers/firmware/efi/efi.c +2 -9
+     }
+
      rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
      if (!rsv)
              return -ENOMEM;
@@ -991,20 +999,6 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
      return 0;
 }
 
-static int __init efi_memreserve_root_init(void)
-{
-     if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
-             return -ENODEV;
-
-     efi_memreserve_root = memremap(efi.mem_reserve,
-                                    sizeof(*efi_memreserve_root),
-                                    MEMREMAP_WB);
-     if (!efi_memreserve_root)
Quoted 9 lines
drivers/firmware/efi/efi.c int efi_mem_reserve_persistent(phys_addr_t addr, u64 size) · line 1006
Quoted 4 lines
drivers/firmware/efi/efi.c +0 -4
-             return -ENOMEM;
-     return 0;
-}
-early_initcall(efi_memreserve_root_init);
But if we keep this (+ a check that the root is indeed NULL), we should be able to make sure efi_memreserve_root is set before we enable a secondary CPU. Still fragile though.

Quoted 2 lines

Why is that stil fragile? It sure isn't pretty, but early initcalls run before SMP boot so it should always work as expected no?

It is just me being paranoid about things happening without locking, but it should indeed work as expected.

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Jan Glauber

2018-11-21 09:56 UTC #4 lore raw

PATCH 1/1 efi: map memreserve table before first use

In reply to Ard Biesheuvel

Quoted 65 lines · Ard Biesheuvel

So instead, move the initialization performed by the initcall into efi_mem_reserve_persistent() itself.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/efi.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

drivers/firmware/efi/efi.c +10 ~~-17~~

index fad7c62cfc0e..40de2f6734cc 100644
@@ -967,15 +967,23 @@ bool efi_is_table_address(unsigned long phys_addr)
 }
 
 static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
-static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
+static struct linux_efi_memreserve *efi_memreserve_root;
 
 int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 {
        struct linux_efi_memreserve *rsv;
 
-       if (!efi_memreserve_root)
+       if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
                return -ENODEV;
 
+       if (!efi_memreserve_root) {
+               efi_memreserve_root = memremap(efi.mem_reserve,
+                                              sizeof(*efi_memreserve_root),
+                                              MEMREMAP_WB);
+               if (WARN_ON_ONCE(!efi_memreserve_root))
+                       return -ENOMEM;
+       }
+
        rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
        if (!rsv)
                return -ENOMEM;

@@ -991,20 +999,6 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
        return 0;
 }
 
-static int __init efi_memreserve_root_init(void)
-{
-       if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
-               return -ENODEV;
-
-       efi_memreserve_root = memremap(efi.mem_reserve,
-                                      sizeof(*efi_memreserve_root),
-                                      MEMREMAP_WB);
-       if (!efi_memreserve_root)
-               return -ENOMEM;
-       return 0;
-}
-early_initcall(efi_memreserve_root_init);
-
 #ifdef CONFIG_KEXEC
 static int update_efi_random_seed(struct notifier_block *nb,
                                  unsigned long code, void *unused)
--

Quoted 1 line

2.17.1

Hi Ard,

thanks for the patch, it works fine for me.

If you want:

Tested-by Jan Glauber <[email protected]>

--Jan

John Garry

2018-11-21 13:33 UTC #5 lore raw

PATCH 1/1 efi: map memreserve table before first use

Quoted 70 lines · Jan Glauber

Quoted 65 lines · Ard Biesheuvel

So instead, move the initialization performed by the initcall into efi_mem_reserve_persistent() itself.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/efi.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

drivers/firmware/efi/efi.c +10 ~~-17~~

index fad7c62cfc0e..40de2f6734cc 100644
@@ -967,15 +967,23 @@ bool efi_is_table_address(unsigned long phys_addr)
 }
 
 static DEFINE_SPINLOCK(efi_mem_reserve_persistent_lock);
-static struct linux_efi_memreserve *efi_memreserve_root __ro_after_init;
+static struct linux_efi_memreserve *efi_memreserve_root;
 
 int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 {
        struct linux_efi_memreserve *rsv;
 
-       if (!efi_memreserve_root)
+       if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
                return -ENODEV;
 
+       if (!efi_memreserve_root) {
+               efi_memreserve_root = memremap(efi.mem_reserve,
+                                              sizeof(*efi_memreserve_root),
+                                              MEMREMAP_WB);
+               if (WARN_ON_ONCE(!efi_memreserve_root))
+                       return -ENOMEM;
+       }
+
        rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
        if (!rsv)
                return -ENOMEM;

@@ -991,20 +999,6 @@ int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
        return 0;
 }
 
-static int __init efi_memreserve_root_init(void)
-{
-       if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
-               return -ENODEV;
-
-       efi_memreserve_root = memremap(efi.mem_reserve,
-                                      sizeof(*efi_memreserve_root),
-                                      MEMREMAP_WB);
-       if (!efi_memreserve_root)
-               return -ENOMEM;
-       return 0;
-}
-early_initcall(efi_memreserve_root_init);
-
 #ifdef CONFIG_KEXEC
 static int update_efi_random_seed(struct notifier_block *nb,
                                  unsigned long code, void *unused)
--

Quoted 1 line

2.17.1

Hi Ard,

Quoted 4 lines

thanks for the patch, it works fine for me.
If you want:
Tested-by Jan Glauber <[email protected]>

FWIW, the splat is gone:

Tested-by John Garry <[email protected]>

Quoted 4 lines

--Jan