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@xxxxxxxxxx>
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@xxxxxxx>
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...
