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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux