Re: [RFT PATCH] efi: map memreserve table before first use

[Marc Zyngier <marc.zyngier@arm.com>]

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...