Re: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]

Re: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]

[Dave Young]

Hello,

On 05/26/17 at 12:49am, Maniaxx wrote:
> PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2)
> [regression]
> 
> Commit:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b0a911478c74ca02581d496f732c10e811e894f
> 
> Kernel 4.10 and below are ok. 4.11 and 4.12rc2.r62.g2426125ab4eb freeze.
> Reverting the commit on 4.11 fixes the problem. Hardware report and
> earlyprintk output on the bugtracker.
> https://bugzilla.kernel.org/show_bug.cgi?id=195633
> 
> earlyprintk (text version):
> https://bbs.archlinux.org/viewtopic.php?pid=1713322#p1713322
> 
> Discussions on Arch Linux Forum:
> https://bbs.archlinux.org/viewtopic.php?pid=1713344
> https://bbs.archlinux.org/viewtopic.php?id=226490
> 

Can you try below patch see if it works for you?
https://lkml.org/lkml/2017/5/15/940

Thanks
Dave

Re: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]

[Dave Young]

Seems efi list was dropped, it if is intended not to cc public list
please let me know.

On 05/26/17 at 03:04pm, Maniaxx wrote:
> On 26.05.2017 at 03:23 wrote Dave Young:
> > Hello,
> >
> > On 05/26/17 at 12:49am, Maniaxx wrote:
> >> PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2)
> >> [regression]
> >>
> >> Commit:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b0a911478c74ca02581d496f732c10e811e894f
> >>
> >> Kernel 4.10 and below are ok. 4.11 and 4.12rc2.r62.g2426125ab4eb freeze.
> >> Reverting the commit on 4.11 fixes the problem. Hardware report and
> >> earlyprintk output on the bugtracker.
> >> https://bugzilla.kernel.org/show_bug.cgi?id=195633
> >>
> >> earlyprintk (text version):
> >> https://bbs.archlinux.org/viewtopic.php?pid=1713322#p1713322
> >>
> >> Discussions on Arch Linux Forum:
> >> https://bbs.archlinux.org/viewtopic.php?pid=1713344
> >> https://bbs.archlinux.org/viewtopic.php?id=226490
> >>
> > Can you try below patch see if it works for you?
> > https://lkml.org/lkml/2017/5/15/940
> >
> > Thanks
> > Dave
> >
> Hello,
> doesn't work. Probably because the condition isn't met (i do have an EFI
> system). At least 4 people reported the problem and if i see it right
> all of them have Asus/Asrock boards. The BGRT init (at least on my side)
> always crashed with an Oops (on 4.10 and below) but never panic'd/froze.

Can you double check it works before the commit of below? If machine
boots fine then check /sys/firmware/acpi/bgrt/image see if it exists or
not. One possible reason is bgrt init code bail out for some reason but
not in new code.

If old code boots fine but without correct /sys/firmware/acpi/bgrt/*
then probably you can debug why it is not created then we can get some
clue about this report.

BTW if you can manange to save full kernel log it would be also helpful.

commit 7b0a911478c74ca02581d496f732c10e811e894f
Author: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date:   Tue Jan 31 13:21:40 2017 +0000

    efi/x86: Move the EFI BGRT init code to early init code

> 
> -Maniaxx
> 

Thanks
Dave

Re: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]

[Dave Young]

On 05/27/17 at 01:14pm, Dave Young wrote:
> Seems efi list was dropped, it if is intended not to cc public list
> please let me know.
> 
> On 05/26/17 at 03:04pm, Maniaxx wrote:
> > On 26.05.2017 at 03:23 wrote Dave Young:
> > > Hello,
> > >
> > > On 05/26/17 at 12:49am, Maniaxx wrote:
> > >> PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2)
> > >> [regression]
> > >>
> > >> Commit:
> > >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7b0a911478c74ca02581d496f732c10e811e894f
> > >>
> > >> Kernel 4.10 and below are ok. 4.11 and 4.12rc2.r62.g2426125ab4eb freeze.
> > >> Reverting the commit on 4.11 fixes the problem. Hardware report and
> > >> earlyprintk output on the bugtracker.
> > >> https://bugzilla.kernel.org/show_bug.cgi?id=195633
> > >>
> > >> earlyprintk (text version):
> > >> https://bbs.archlinux.org/viewtopic.php?pid=1713322#p1713322
> > >>
> > >> Discussions on Arch Linux Forum:
> > >> https://bbs.archlinux.org/viewtopic.php?pid=1713344
> > >> https://bbs.archlinux.org/viewtopic.php?id=226490
> > >>
> > > Can you try below patch see if it works for you?
> > > https://lkml.org/lkml/2017/5/15/940
> > >
> > > Thanks
> > > Dave
> > >
> > Hello,
> > doesn't work. Probably because the condition isn't met (i do have an EFI
> > system). At least 4 people reported the problem and if i see it right
> > all of them have Asus/Asrock boards. The BGRT init (at least on my side)
> > always crashed with an Oops (on 4.10 and below) but never panic'd/froze.
> 
> Can you double check it works before the commit of below? If machine
> boots fine then check /sys/firmware/acpi/bgrt/image see if it exists or
> not. One possible reason is bgrt init code bail out for some reason but
> not in new code.
> 
> If old code boots fine but without correct /sys/firmware/acpi/bgrt/*
> then probably you can debug why it is not created then we can get some
> clue about this report.

Also it would be better to print the bgrt table like below (copied from
Sabrina Dubroca's test code in another mail thread):
        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);

> 
> BTW if you can manange to save full kernel log it would be also helpful.
> 
> commit 7b0a911478c74ca02581d496f732c10e811e894f
> Author: Dave Young <dyoung-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Date:   Tue Jan 31 13:21:40 2017 +0000
> 
>     efi/x86: Move the EFI BGRT init code to early init code
> 
> > 
> > -Maniaxx
> > 
> 
> Thanks
> Dave

Re: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]

[Maniaxx]

On 27.05.2017 at 07:34 wrote Dave Young:
>> Can you double check it works before the commit of below? 
linux-git-4.10rc5.r115.gc4c39c70c5fe is fine (1 commit back)
linux-git-4.10rc5.r116.g7b0a911478c7 is not (early init BGRT patch)

>> If machine
>> boots fine then check /sys/firmware/acpi/bgrt/image see if it exists or
>> not. 
Not sure. /sys/firmware/acpi/bgrt/ doesn't exist here but i have
/sys/firmware/acpi/tables/ that contains all tables. BGRT is 60 bytes
and holds the same binary content as pr_info hex_dump (in dmesg log below).

>> if you can manange to save full kernel log it would be also helpful.
Here is a log of linux-git-4.10rc5.r115.gc4c39c70c5fe with Sabrina
Dubroca's test code applied:
https://pastebin.com/ZkBNejrC

This is how i applied the patch. I hope its correct:
https://pastebin.com/see6V0Bk

Re: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]

[Dave Young]

Cc ACPI people for thoughts..

On 06/01/17 at 03:17am, Maniaxx wrote:
> On 27.05.2017 at 07:34 wrote Dave Young:
> >> Can you double check it works before the commit of below? 
> linux-git-4.10rc5.r115.gc4c39c70c5fe is fine (1 commit back)
> linux-git-4.10rc5.r116.g7b0a911478c7 is not (early init BGRT patch)
> 
> >> If machine
> >> boots fine then check /sys/firmware/acpi/bgrt/image see if it exists or
> >> not. 
> Not sure. /sys/firmware/acpi/bgrt/ doesn't exist here but i have
> /sys/firmware/acpi/tables/ that contains all tables. BGRT is 60 bytes
> and holds the same binary content as pr_info hex_dump (in dmesg log below).

This means the efi_bgrt_init failed out originally before the early init BGRT
patch. Checking the code the only difference is current code we have no
below code:

        status = acpi_get_table("BGRT", 0,
                                (struct acpi_table_header **)&bgrt_tab);
        if (ACPI_FAILURE(status))
                return;

So probably acpi_get_table has some more sanity checking and it failed
early. Can you add a printk before above return to confirm it? Just test
the old kernel without early init BGRT patch.

> 
> >> if you can manange to save full kernel log it would be also helpful.
> Here is a log of linux-git-4.10rc5.r115.gc4c39c70c5fe with Sabrina
> Dubroca's test code applied:
> https://pastebin.com/ZkBNejrC
> 
> This is how i applied the patch. I hope its correct:
> https://pastebin.com/see6V0Bk
> 

Thanks
Dave

Re: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]

[Maniaxx]

On 01.06.2017 at 03:57 wrote Dave Young:
> This means the efi_bgrt_init failed out originally before the early init BGRT
> patch. Checking the code the only difference is current code we have no
> below code:
> 
>         status = acpi_get_table("BGRT", 0,
>                                 (struct acpi_table_header **)&bgrt_tab);
>         if (ACPI_FAILURE(status))
>                 return;
> 
> So probably acpi_get_table has some more sanity checking and it failed
> early. Can you add a printk before above return to confirm it? Just test
> the old kernel without early init BGRT patch.

Doesn't fail early. I can see the printk.

Re: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]

[Dave Young]

On 06/01/17 at 09:13pm, Maniaxx wrote:
> On 01.06.2017 at 03:57 wrote Dave Young:
> > This means the efi_bgrt_init failed out originally before the early init BGRT
> > patch. Checking the code the only difference is current code we have no
> > below code:
> > 
> >         status = acpi_get_table("BGRT", 0,
> >                                 (struct acpi_table_header **)&bgrt_tab);
> >         if (ACPI_FAILURE(status))
> >                 return;
> > 
> > So probably acpi_get_table has some more sanity checking and it failed
> > early. Can you add a printk before above return to confirm it? Just test
> > the old kernel without early init BGRT patch.
> 
> Doesn't fail early. I can see the printk.

Since you do not see /sys/firmware/acpi/bgrt, that means there must be
somewhere failed early so bgrt image sysfs files are not created.
for old kernel arch/x86/platform/efi/efi-bgrt.c copy the bgrt image
then drivers/acpi/bgrt.c will populate the image in sysfs.

Can you do more debugging ie. adding more printk see why bgrt image
sysfs dir is not created?

If we get the reason then we can start to see what we missed in new
code.

Thanks
Dave

Re: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]

[Maniaxx]

Isn't the image address invalid if bgrt_tab->status is 0
(regarding ACPI specs below)?
The image address could be total garbage and probably the
reason why memremap (in efi-bgrt.c) leads to different 
behavior on early_init.

If so, you might need to add another condition to check
status and bail out before memremap.

**************************************************************************************
Source: http://www.acpi.info/DOWNLOADS/ACPIspec50.pdf

5.2.22 Boot Graphics Resource Table (BGRT)
[...]
If the boot path is interrupted (e.g. by a key press), 
the valid bit within the status field should be changed to 0 
to indicate to the OS that the current image is invalidated.

5.2.22.2 Status
The status field contains information about the current 
status of the table.  The Valid bit is bit 0 of the lowest byte. 
It should be set to 1 when the table is written, 
and invalidated if there is reason to expect that the screen 
state has been changed.

5.2.22.4 Image Address
The Image Address contains the location in memory where an in-memory 
copy of the boot image can be found.
The image should be stored in EfiBootServicesData, allowing the system 
to reclaim the memory when the image is no longer needed.

**************************************************************************************
Decompiled BGRT table:

$ sudo acpidump > acpi.dat
$ acpixtract -a acpi.dat
$ iasl -d bgrt.dat 
$ cat bgrt.dsl

/*
 * Intel ACPI Component Architecture
 * AML/ASL+ Disassembler version 20170303 (64-bit version)
 * Copyright (c) 2000 - 2017 Intel Corporation
 * 
 * Disassembly of bgrt.dat, Sat Jun  3 15:33:10 2017
 *
 * ACPI Data Table [BGRT]
 *
 * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
 */

[000h 0000   4]                    Signature : "BGRT"    [Boot Graphics Resource Table]
[004h 0004   4]                 Table Length : 0000003C
[008h 0008   1]                     Revision : 00
[009h 0009   1]                     Checksum : 89
[00Ah 0010   6]                       Oem ID : "ALASKA"
[010h 0016   8]                 Oem Table ID : "A M I"
[018h 0024   4]                 Oem Revision : 01072009
[01Ch 0028   4]              Asl Compiler ID : "AMI "
[020h 0032   4]        Asl Compiler Revision : 00010013

[024h 0036   2]                      Version : 0001
[026h 0038   1]                       Status : 00
[027h 0039   1]                   Image Type : 00
[028h 0040   8]                Image Address : 0A08E01800000001
[030h 0048   4]                Image OffsetX : 00000000
[034h 0052   4]                Image OffsetY : 00000000

Raw Table Data: Length 60 (0x3C)

  0000: 42 47 52 54 3C 00 00 00 00 89 41 4C 41 53 4B 41  // BGRT<.....ALASKA
  0010: 41 20 4D 20 49 00 00 00 09 20 07 01 41 4D 49 20  // A M I.... ..AMI 
  0020: 13 00 01 00 01 00 00 00 01 00 00 00 18 E0 08 0A  // ................
  0030: 00 00 00 00 00 00 00 00 00 00 00 00              // ............

Re: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]

[Maniaxx]

New kernel (with early_init panic):
In contrast to old kernel early_memremap is successful and
"Ignoring BGRT: failed to map image header memory" condition
isn't triggered.
https://github.com/torvalds/linux/blob/v4.11-rc1/arch/x86/platform/efi/efi-bgrt.c#L65

The next memcpy is the culprit for the panic. I've checked that with printk.
https://github.com/torvalds/linux/blob/v4.11-rc1/arch/x86/platform/efi/efi-bgrt.c#L71

This leads to this commit:
https://github.com/torvalds/linux/commit/66dbe99cfe30e113d2e571e68b9b6a1a8985a157

My u64 image address might be complete garbage (0A08E01800000001). Rather looks like 
2x u32. Here is another BGRT image address that looks much more valid than mine 
(on boundary, lower mem area).
https://ubuntuforums.org/showthread.php?t=2264380

If so, we are poking in invalid memory areas.

Reverting the commit until a better way is found to identify a valid
image address should fix the problem.

Re: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]

[Maniaxx]

I've reverted 66dbe99cfe30e113d2e571e68b9b6a1a8985a157 from 
linux-git-4.11.r1.gaf8b4c0323e1 and it boots properly now indeed.

I've asked for some more BGRT dumps to get more image addresses:
https://bbs.archlinux.org/viewtopic.php?pid=1715699#p1715699

Re: PROBLEM: Kernel panic on EFI BGRT early init code (kernel 4.11/4.12rc2) [regression]

[Dave Young]

On 06/02/17 at 10:51am, Dave Young wrote:
> On 06/01/17 at 09:13pm, Maniaxx wrote:
> > On 01.06.2017 at 03:57 wrote Dave Young:
> > > This means the efi_bgrt_init failed out originally before the early init BGRT
> > > patch. Checking the code the only difference is current code we have no
> > > below code:
> > > 
> > >         status = acpi_get_table("BGRT", 0,
> > >                                 (struct acpi_table_header **)&bgrt_tab);
> > >         if (ACPI_FAILURE(status))
> > >                 return;
> > > 
> > > So probably acpi_get_table has some more sanity checking and it failed
> > > early. Can you add a printk before above return to confirm it? Just test
> > > the old kernel without early init BGRT patch.
> > 
> > Doesn't fail early. I can see the printk.
> 
> Since you do not see /sys/firmware/acpi/bgrt, that means there must be
> somewhere failed early so bgrt image sysfs files are not created.
> for old kernel arch/x86/platform/efi/efi-bgrt.c copy the bgrt image
> then drivers/acpi/bgrt.c will populate the image in sysfs.
> 
> Can you do more debugging ie. adding more printk see why bgrt image
> sysfs dir is not created?
> 
> If we get the reason then we can start to see what we missed in new
> code.

Please ignore the request, I observed in your old kernel boot log there
is a ioremap error message about invalid physical address, so that is
clear since ioremap failed then efi_bgrt_init bails out.

In new code we use early_memremap, it seems lacks of checking invalid
physical address, The firmware provides garbage in efi bgrt image
addresses, I will work on a fix for this. 

Thanks
Dave