All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFT PATCH] efi: map memreserve table before first use
@ 2018-11-20 17:35 ` Ard Biesheuvel
  0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2018-11-20 17:35 UTC (permalink / raw)
  To: linux-efi
  Cc: marc.zyngier, john.garry, Ard Biesheuvel, linux-arm-kernel, Jan.Glauber

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 <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/efi.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index fad7c62cfc0e..40de2f6734cc 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -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)
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [RFT PATCH] efi: map memreserve table before first use
@ 2018-11-20 17:35 ` Ard Biesheuvel
  0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2018-11-20 17:35 UTC (permalink / raw)
  To: linux-arm-kernel

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 <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/efi.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index fad7c62cfc0e..40de2f6734cc 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -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)
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [RFT PATCH] efi: map memreserve table before first use
  2018-11-20 17:35 ` Ard Biesheuvel
@ 2018-11-20 18:29   ` Marc Zyngier
  -1 siblings, 0 replies; 12+ messages in thread
From: Marc Zyngier @ 2018-11-20 18:29 UTC (permalink / raw)
  To: Ard Biesheuvel, linux-efi; +Cc: john.garry, linux-arm-kernel, Jan.Glauber

Hi Ard,

On 20/11/2018 17:35, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel@linaro.org>

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

Tested-by: Marc Zyngier <marc.zyngier@arm.com>

A comment below though:

> ---
>  drivers/firmware/efi/efi.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index fad7c62cfc0e..40de2f6734cc 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -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;

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

> +	}
> +
>  	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);

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.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [RFT PATCH] efi: map memreserve table before first use
@ 2018-11-20 18:29   ` Marc Zyngier
  0 siblings, 0 replies; 12+ messages in thread
From: Marc Zyngier @ 2018-11-20 18:29 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Ard,

On 20/11/2018 17:35, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel@linaro.org>

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

Tested-by: Marc Zyngier <marc.zyngier@arm.com>

A comment below though:

> ---
>  drivers/firmware/efi/efi.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index fad7c62cfc0e..40de2f6734cc 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -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;

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

> +	}
> +
>  	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);

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.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [RFT PATCH] efi: map memreserve table before first use
  2018-11-20 18:29   ` Marc Zyngier
@ 2018-11-20 18:35     ` Ard Biesheuvel
  -1 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2018-11-20 18:35 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: John Garry, linux-efi, linux-arm-kernel, Jan.Glauber

On Tue, 20 Nov 2018 at 19:29, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> Hi Ard,
>
> On 20/11/2018 17:35, Ard Biesheuvel wrote:
> > 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 <ard.biesheuvel@linaro.org>
>
> I've just given it a go on one of my TX2s, and it boots just fine. So
> for that:
>
> Tested-by: Marc Zyngier <marc.zyngier@arm.com>
>
> A comment below though:
>
> > ---
> >  drivers/firmware/efi/efi.c | 26 ++++++++++----------------
> >  1 file changed, 10 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index fad7c62cfc0e..40de2f6734cc 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -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;
>
> This is now a bit racy if there is more than a single online CPU.
>
> > +     }
> > +
> >       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);
>
> 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?

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [RFT PATCH] efi: map memreserve table before first use
@ 2018-11-20 18:35     ` Ard Biesheuvel
  0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2018-11-20 18:35 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, 20 Nov 2018 at 19:29, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> Hi Ard,
>
> On 20/11/2018 17:35, Ard Biesheuvel wrote:
> > 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 <ard.biesheuvel@linaro.org>
>
> I've just given it a go on one of my TX2s, and it boots just fine. So
> for that:
>
> Tested-by: Marc Zyngier <marc.zyngier@arm.com>
>
> A comment below though:
>
> > ---
> >  drivers/firmware/efi/efi.c | 26 ++++++++++----------------
> >  1 file changed, 10 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index fad7c62cfc0e..40de2f6734cc 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -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;
>
> This is now a bit racy if there is more than a single online CPU.
>
> > +     }
> > +
> >       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);
>
> 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?

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [RFT PATCH] efi: map memreserve table before first use
  2018-11-20 18:35     ` Ard Biesheuvel
@ 2018-11-20 18:44       ` Marc Zyngier
  -1 siblings, 0 replies; 12+ messages in thread
From: Marc Zyngier @ 2018-11-20 18:44 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: John Garry, linux-efi, linux-arm-kernel, Jan.Glauber

On 20/11/2018 18:35, Ard Biesheuvel wrote:
> On Tue, 20 Nov 2018 at 19:29, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>
>> Hi Ard,
>>
>> On 20/11/2018 17:35, Ard Biesheuvel wrote:
>>> 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 <ard.biesheuvel@linaro.org>
>>
>> I've just given it a go on one of my TX2s, and it boots just fine. So
>> for that:
>>
>> Tested-by: Marc Zyngier <marc.zyngier@arm.com>
>>
>> A comment below though:
>>
>>> ---
>>>  drivers/firmware/efi/efi.c | 26 ++++++++++----------------
>>>  1 file changed, 10 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>> index fad7c62cfc0e..40de2f6734cc 100644
>>> --- a/drivers/firmware/efi/efi.c
>>> +++ b/drivers/firmware/efi/efi.c
>>> @@ -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;
>>
>> This is now a bit racy if there is more than a single online CPU.
>>
>>> +     }
>>> +
>>>       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);
>>
>> 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?

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

Thanks,

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [RFT PATCH] efi: map memreserve table before first use
@ 2018-11-20 18:44       ` Marc Zyngier
  0 siblings, 0 replies; 12+ messages in thread
From: Marc Zyngier @ 2018-11-20 18:44 UTC (permalink / raw)
  To: linux-arm-kernel

On 20/11/2018 18:35, Ard Biesheuvel wrote:
> On Tue, 20 Nov 2018 at 19:29, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>
>> Hi Ard,
>>
>> On 20/11/2018 17:35, Ard Biesheuvel wrote:
>>> 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 <ard.biesheuvel@linaro.org>
>>
>> I've just given it a go on one of my TX2s, and it boots just fine. So
>> for that:
>>
>> Tested-by: Marc Zyngier <marc.zyngier@arm.com>
>>
>> A comment below though:
>>
>>> ---
>>>  drivers/firmware/efi/efi.c | 26 ++++++++++----------------
>>>  1 file changed, 10 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>> index fad7c62cfc0e..40de2f6734cc 100644
>>> --- a/drivers/firmware/efi/efi.c
>>> +++ b/drivers/firmware/efi/efi.c
>>> @@ -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;
>>
>> This is now a bit racy if there is more than a single online CPU.
>>
>>> +     }
>>> +
>>>       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);
>>
>> 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?

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

Thanks,

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [RFT PATCH] efi: map memreserve table before first use
  2018-11-20 17:35 ` Ard Biesheuvel
@ 2018-11-21  9:56   ` Jan Glauber
  -1 siblings, 0 replies; 12+ messages in thread
From: Jan Glauber @ 2018-11-21  9:56 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: marc.zyngier, john.garry, linux-efi, linux-arm-kernel

On Tue, Nov 20, 2018 at 06:35:42PM +0100, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel@linaro.org>
> ---
>  drivers/firmware/efi/efi.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index fad7c62cfc0e..40de2f6734cc 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -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)
> --
> 2.17.1

Hi Ard,

thanks for the patch, it works fine for me.

If you want:
Tested-by: Jan Glauber <jglauber@cavium.com>

--Jan

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [RFT PATCH] efi: map memreserve table before first use
@ 2018-11-21  9:56   ` Jan Glauber
  0 siblings, 0 replies; 12+ messages in thread
From: Jan Glauber @ 2018-11-21  9:56 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Nov 20, 2018 at 06:35:42PM +0100, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel@linaro.org>
> ---
>  drivers/firmware/efi/efi.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index fad7c62cfc0e..40de2f6734cc 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -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)
> --
> 2.17.1

Hi Ard,

thanks for the patch, it works fine for me.

If you want:
Tested-by: Jan Glauber <jglauber@cavium.com>

--Jan

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [RFT PATCH] efi: map memreserve table before first use
  2018-11-21  9:56   ` Jan Glauber
@ 2018-11-21 13:33     ` John Garry
  -1 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2018-11-21 13:33 UTC (permalink / raw)
  To: Jan Glauber, Ard Biesheuvel; +Cc: marc.zyngier, linux-efi, linux-arm-kernel

On 21/11/2018 09:56, Jan Glauber wrote:
> On Tue, Nov 20, 2018 at 06:35:42PM +0100, Ard Biesheuvel wrote:
>> 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 <ard.biesheuvel@linaro.org>
>> ---
>>  drivers/firmware/efi/efi.c | 26 ++++++++++----------------
>>  1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>> index fad7c62cfc0e..40de2f6734cc 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c
>> @@ -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)
>> --
>> 2.17.1
>
> Hi Ard,
>
> thanks for the patch, it works fine for me.
>
> If you want:
> Tested-by: Jan Glauber <jglauber@cavium.com>

FWIW, the splat is gone:

Tested-by: John Garry <john.garry@huawei.com>

>
> --Jan
>
>

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [RFT PATCH] efi: map memreserve table before first use
@ 2018-11-21 13:33     ` John Garry
  0 siblings, 0 replies; 12+ messages in thread
From: John Garry @ 2018-11-21 13:33 UTC (permalink / raw)
  To: linux-arm-kernel

On 21/11/2018 09:56, Jan Glauber wrote:
> On Tue, Nov 20, 2018 at 06:35:42PM +0100, Ard Biesheuvel wrote:
>> 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 <ard.biesheuvel@linaro.org>
>> ---
>>  drivers/firmware/efi/efi.c | 26 ++++++++++----------------
>>  1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>> index fad7c62cfc0e..40de2f6734cc 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c
>> @@ -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)
>> --
>> 2.17.1
>
> Hi Ard,
>
> thanks for the patch, it works fine for me.
>
> If you want:
> Tested-by: Jan Glauber <jglauber@cavium.com>

FWIW, the splat is gone:

Tested-by: John Garry <john.garry@huawei.com>

>
> --Jan
>
>

^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-11-21 13:33 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-20 17:35 [RFT PATCH] efi: map memreserve table before first use Ard Biesheuvel
2018-11-20 17:35 ` Ard Biesheuvel
2018-11-20 18:29 ` Marc Zyngier
2018-11-20 18:29   ` Marc Zyngier
2018-11-20 18:35   ` Ard Biesheuvel
2018-11-20 18:35     ` Ard Biesheuvel
2018-11-20 18:44     ` Marc Zyngier
2018-11-20 18:44       ` Marc Zyngier
2018-11-21  9:56 ` Jan Glauber
2018-11-21  9:56   ` Jan Glauber
2018-11-21 13:33   ` John Garry
2018-11-21 13:33     ` John Garry

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.