On Tue, 20 Nov 2018 at 19:29, Marc Zyngier <marc.zyngier@xxxxxxx> 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@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.
>
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?
