Re: [PATCH 08/10] efi/x86: Move EFI BGRT init code to early init code

[Dave Young <dyoung@redhat.com>]

On 05/15/17 at 01:10pm, Sabrina Dubroca wrote:
> 2017-05-15, 16:37:40 +0800, Dave Young wrote:
> > Hi,
> > 
> > Thanks for the report.
> > On 05/14/17 at 01:18am, Sabrina Dubroca wrote:
> > > 2017-01-31, 13:21:40 +0000, Ard Biesheuvel wrote:
> > > > From: Dave Young <dyoung@redhat.com>
> > > > 
> > > > Before invoking the arch specific handler, efi_mem_reserve() reserves
> > > > the given memory region through memblock.
> > > > 
> > > > efi_bgrt_init() will call efi_mem_reserve() after mm_init(), at which
> > > > time memblock is dead and should not be used anymore.
> > > > 
> > > > The EFI BGRT code depends on ACPI initialization to get the BGRT ACPI
> > > > table, so move parsing of the BGRT table to ACPI early boot code to
> > > > ensure that efi_mem_reserve() in EFI BGRT code still use memblock safely.
> > > > 
> > > > Signed-off-by: Dave Young <dyoung@redhat.com>
> > > > Cc: Matt Fleming <matt@codeblueprint.co.uk>
> > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > > > Cc: Len Brown <lenb@kernel.org>
> > > > Cc: linux-acpi@vger.kernel.org
> > > > Tested-by: Bhupesh Sharma <bhsharma@redhat.com>
> > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > 
> > > I have a box that panics in early boot after this patch. The kernel
> > > config is based on a Fedora 25 kernel + localmodconfig.
> > > 
> > > BUG: unable to handle kernel paging request at ffffffffff240001
> > > IP: efi_bgrt_init+0xdc/0x134
> > > PGD 1ac0c067
> > > PUD 1ac0e067
> > > PMD 1aee9067
> > > PTE 9380701800000163
> > > 
> > > Oops: 0009 [#1] SMP
> > > Modules linked in:
> > > CPU: 0 PID: 0 Comm: swapper Not tainted 4.10.0-rc5-00116-g7b0a911 #19
> > > Hardware name: Hewlett-Packard HP Z220 CMT Workstation/1790, BIOS K51 v01.02 05/03/2012
> > > task: ffffffff9fc10500 task.stack: ffffffff9fc00000
> > > RIP: 0010:efi_bgrt_init+0xdc/0x134
> > > RSP: 0000:ffffffff9fc03d58 EFLAGS: 00010082
> > > RAX: ffffffffff240001 RBX: 0000000000000000 RCX: 1380701800000006
> > > RDX: 8000000000000163 RSI: 9380701800000163 RDI: 00000000000005be
> > > RBP: ffffffff9fc03d70 R08: 1380701800001000 R09: 0000000000000002
> > > R10: 000000000002d000 R11: ffff98a3dedd2fc6 R12: ffffffff9f9f22b6
> > > R13: ffffffff9ff49480 R14: 0000000000000010 R15: 0000000000000000
> > > FS:  0000000000000000(0000) GS:ffffffff9fd20000(0000) knlGS:0000000000000000
> > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: ffffffffff240001 CR3: 000000001ac09000 CR4: 00000000000406b0
> > > Call Trace:
> > >  ? acpi_parse_ioapic+0x98/0x98
> > >  acpi_parse_bgrt+0x9/0xd
> > >  acpi_table_parse+0x7a/0xa9
> > >  acpi_boot_init+0x3c7/0x4f9
> > >  ? acpi_parse_x2apic+0x74/0x74
> > >  ? acpi_parse_x2apic_nmi+0x46/0x46
> > >  setup_arch+0xb4b/0xc6f
> > >  ? printk+0x52/0x6e
> > >  start_kernel+0xb2/0x47b
> > >  ? early_idt_handler_array+0x120/0x120
> > >  x86_64_start_reservations+0x24/0x26
> > >  x86_64_start_kernel+0xf7/0x11a
> > >  start_cpu+0x14/0x14
> > > Code: 48 c7 c7 10 16 a0 9f e8 4e 94 40 ff eb 62 be 06 00 00 00 e8 f9 ff 00 00 48 85 c0 75 0e 48 c7 c7 40 16 a0 9f e8 31 94 40 ff eb 45 <66> 44 8b 20 be 06 00 00 00 48 89 c7 8b 58 02 e8 87 00 01 00 66
> > > RIP: efi_bgrt_init+0xdc/0x134 RSP: ffffffff9fc03d58
> > > CR2: ffffffffff240001
> > > ---[ end trace f68728a0d3053b52 ]---
> > > Kernel panic - not syncing: Attempted to kill the idle task!
> > > ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
> > > 
> > > 
> > > That code is:
> > > 
> > > 
> > > All code
> > > ========
> > >    0:	48 c7 c7 10 16 a0 9f 	mov    $0xffffffff9fa01610,%rdi
> > >    7:	e8 4e 94 40 ff       	callq  0xffffffffff40945a
> > >    c:	eb 62                	jmp    0x70
> > >    e:	be 06 00 00 00       	mov    $0x6,%esi
> > >   13:	e8 f9 ff 00 00       	callq  0x10011
> > >   18:	48 85 c0             	test   %rax,%rax
> > >   1b:	75 0e                	jne    0x2b
> > >   1d:	48 c7 c7 40 16 a0 9f 	mov    $0xffffffff9fa01640,%rdi
> > >   24:	e8 31 94 40 ff       	callq  0xffffffffff40945a
> > >   29:	eb 45                	jmp    0x70
> > >   2b:*	66 44 8b 20          	mov    (%rax),%r12w		<-- trapping instruction
> > >   2f:	be 06 00 00 00       	mov    $0x6,%esi
> > >   34:	48 89 c7             	mov    %rax,%rdi
> > >   37:	8b 58 02             	mov    0x2(%rax),%ebx
> > >   3a:	e8 87 00 01 00       	callq  0x100c6
> > >   3f:	66                   	data16
> > > 
> > > Code starting with the faulting instruction
> > > ===========================================
> > >    0:	66 44 8b 20          	mov    (%rax),%r12w
> > >    4:	be 06 00 00 00       	mov    $0x6,%esi
> > >    9:	48 89 c7             	mov    %rax,%rdi
> > >    c:	8b 58 02             	mov    0x2(%rax),%ebx
> > >    f:	e8 87 00 01 00       	callq  0x1009b
> > >   14:	66                   	data16
> > > 
> > > 
> > > which is just after the early_memremap() call.
> > > 
> > > I enabled early_ioremap_debug and the last warning had:
> > > 
> > > __early_ioremap(1380701800001000, 00001000) [1] => 00000001 + ffffffffff240000
> > 
> > The phys addr looks odd..
> > 
> > From the kernel log, I do not see any efi messages so can you check if
> > you are booting with legacy mode or efi boot?
> 
> I don't have physical access to the machine, but even from a succesful
> boot there's no efi message. No /sys/firmware/efi as well, and
> efivarfs isn't registered despite it being compiled in
> (on kernel 4.10.14-200.fc25.x86_64):
> 
>     # mount -t efivarfs none /mnt/foo
>     mount: unknown filesystem type 'efivarfs'
> 
> So I suppose it's legacy mode and the
>     !efi_enabled(EFI_RUNTIME_SERVICES)
> check kicking in.
> 
> 
> > I suppose bgrt are efi only, if you are test with legacy boot it is odd
> > that there is BGRT table populated.
> > 
> > For debugging purpose maybe you can add some printk to dump the acpi
> > table header in efi_bgrt_init function, just print the version, status,
> > image_type, image_address.
> 
> Added:
> 
>        pr_info("%s acpi_table_bgrt.version %hu\n", __func__, bgrt->version);
>        pr_info("%s acpi_table_bgrt.status %hhu\n", __func__, bgrt->status);
>        pr_info("%s acpi_table_bgrt.image_type %hhu\n", __func__, bgrt->image_type);
>        pr_info("%s acpi_table_bgrt.image_address %llx\n", __func__, bgrt->image_address);
>        print_hex_dump(KERN_INFO, "efi_bgrt_init acpi_table_bgrt", DUMP_PREFIX_OFFSET, 16, 1, bgrt, sizeof(*bgrt), false);
> 
> efi_bgrt: efi_bgrt_init acpi_table_bgrt.version 1
> efi_bgrt: efi_bgrt_init acpi_table_bgrt.status 0
> efi_bgrt: efi_bgrt_init acpi_table_bgrt.image_type 0
> efi_bgrt: efi_bgrt_init acpi_table_bgrt.image_address 1380701800000001
> efi_bgrt_init acpi_table_bgrt: 00000000: 42 47 52 54 3c 00 00 00 00 8b 48 50 51 4f 45 4d
> efi_bgrt_init acpi_table_bgrt: 00000010: 53 4c 49 43 2d 57 4b 53 09 20 07 01 41 4d 49 20
> efi_bgrt_init acpi_table_bgrt: 00000020: 13 00 01 00 01 00 00 00 01 00 00 00 18 70 80 13
> efi_bgrt_init acpi_table_bgrt: 00000030: 00 00 00 00 ff 00 00 00
> 
> 
> > If you can prove it is a non-efi boot, then maybe you can test below
> > patch:
> 
> Yeah, that works.  I guess that makes sense, since before this patch,
> efi_bgrt_init() wasn't called on that box (because of the
> EFI_RUNTIME_SERVICES check in start_kernel()).

Ok, thanks for the testing, from your debug log, it proved this is the
root cause.

> 
> Thanks!
> 
> > diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
> > index 04ca876..b986e26 100644
> > --- a/arch/x86/platform/efi/efi-bgrt.c
> > +++ b/arch/x86/platform/efi/efi-bgrt.c
> > @@ -36,6 +36,9 @@ void __init efi_bgrt_init(struct acpi_table_header *table)
> >  	if (acpi_disabled)
> >  		return;
> >  
> > +	if (!efi_enabled(EFI_CONFIG_TABLES))

A better version should be checking EFI_BOOT, could you retest with
below instead? If it works I can send a patch with your Tested-by:
if (!efi_enabled(EFI_BOOT))

> > +		return;
> > +
> >  	if (table->length < sizeof(bgrt_tab)) {
> >  		pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
> >  		       table->length, sizeof(bgrt_tab));
> > 
> 
> -- 
> Sabrina

Thanks
Dave