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]

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