On 11/12/18 at 3:32 AM, Marc Zyngier wrote:
> On Mon, 12 Nov 2018 02:45:48 +0000,
> Qian Cai <cai@xxxxxx> wrote:
> >
> >
> >
> > > On Nov 9, 2018, at 9:45 PM, Qian Cai <cai@xxxxxx> wrote:
> > >
> > >
> > > On 11/8/18 at 1:05 PM, Ard Biesheuvel wrote:
> > >
> > >> Currently, efi_mem_reserve_persistent() may not be called from atomic
> > >> context, since both the kmalloc() call and the memremap() call may
> > >> sleep.
> > >>
> > >> The kmalloc() call is easy enough to fix, but the memremap() call
> > >> needs to be moved into an init hook since we cannot control the
> > >> memory allocation behavior of memremap() at the call site.
> > >>
> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> > >> ---
> > >> drivers/firmware/efi/efi.c | 31 +++++++++++++++++++------------
> > >> 1 file changed, 19 insertions(+), 12 deletions(-)
> > >>
> > >> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > >> index 249eb70691b0..cfc876e0b67b 100644
> > >> --- a/drivers/firmware/efi/efi.c
> > >> +++ b/drivers/firmware/efi/efi.c
> > >> @@ -963,36 +963,43 @@ 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;
> > >>
> > >> int efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
> > >> {
> > >> - struct linux_efi_memreserve *rsv, *parent;
> > >> + struct linux_efi_memreserve *rsv;
> > >>
> > >> - if (efi.mem_reserve == EFI_INVALID_TABLE_ADDR)
> > >> + if (!efi_memreserve_root)
> > >> return -ENODEV;
> > >>
> > >> - rsv = kmalloc(sizeof(*rsv), GFP_KERNEL);
> > >> + rsv = kmalloc(sizeof(*rsv), GFP_ATOMIC);
> > >> if (!rsv)
> > >> return -ENOMEM;
> > >>
> > >> - parent = memremap(efi.mem_reserve, sizeof(*rsv), MEMREMAP_WB);
> > >> - if (!parent) {
> > >> - kfree(rsv);
> > >> - return -ENOMEM;
> > >> - }
> > >> -
> > >> rsv->base = addr;
> > >> rsv->size = size;
> > >>
> > >> spin_lock(&efi_mem_reserve_persistent_lock);
> > >> - rsv->next = parent->next;
> > >> - parent->next = __pa(rsv);
> > >> + rsv->next = efi_memreserve_root->next;
> > >> + efi_memreserve_root->next = __pa(rsv);
> > >> spin_unlock(&efi_mem_reserve_persistent_lock);
> > >>
> > >> - memunmap(parent);
> > >> + 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,
> > >> --
> > >> 2.19.1
> > > BTW, I won’t be able to apply this patch on top of this series [1]. After applied that series, the original BUG sleep from atomic is gone as well as two other GIC warnings. Do you think a new patch is needed here?
> > >
> > > [1] https://www.spinics.net/lists/arm-kernel/msg685751.html
> > OK, I was able to apply this patch on top of latest mainline (ccda4af0f4b9)
> > which also include one patch (1/6) from the above series,
> >
> > However, the efi-related patches from the series (4/6, 5/6, and 6/6) are no
> > longer able to be cleanly applied.
> >
> > As the results, the above patch did fix the original BUG: sleep from atomic,
> > but it introduces 2 new warnings.
>
> [...]
>
> These are the warnings you've already reported, aren't they? And we've
> established that if you apply the whole series, everything work as
> intended at least on the GIC side (the timer issue is a different
> story altogether).
>
> Or am I missing something?
The problem is that I am not able to apply the whole
series alone on top of the latest mainline (rc2) now to
verify it. Also, I won’t be able to apply the series and
this patch together on top of rc1. There are conflicts
between this patch and 4-6 of the series.
Originally, I said those GIC warnings are gone when
testing rc1 + the series, but I am not sure if it is just
dumb luck that also fix BUG: sleep from atomic. Make
sense?
