Patchwork acpi: bgrt: parse BGRT to obtain BMP address before it gets clobbered

login
register
mail settings
Submitter Ard Biesheuvel
Date Jan. 22, 2019, 3:06 p.m.
Message ID <20190122150616.850-1-ard.biesheuvel@linaro.org>
Download mbox | patch
Permalink /patch/706661/
State New
Headers show

Comments

Ard Biesheuvel - Jan. 22, 2019, 3:06 p.m.
The bitmap left in the framebuffer by the firmware is described by an
ACPI table called "BGRT", which describes the size, pixel format and
the address of a BMP image in memory. While the BGRT ACPI table is
guaranteed to reside in a "ACPI reclaim" memory region, which is
never touched by Linux. The BMP image, however, typically resides
in EFI Boot Services Memory, which may have been overwritten by the
time the BGRT discovery routine runs.

So instead, drop the handling from the ACPI init code, and call the
BGRT parsing code immediately after going over the EFI configuration
table array, at which time no memory has been touched yet except for
the .data/.bss regions covered by the static kernel image.

Unfortunately, this involves a non-trivial amount of ACPI entry
point and root table parsing, but we cannot rely on the normal
ACPI infrastructure yet this early in the boot.

Also note that we cannot take the 'acpi_disabled' global variable
into account, since it may not have assumed the correct value yet
(on arm64, the default value is '1' which is overridden to '0' if
no DT description has been made available by the firmware)

Cc: Peter Jones <pjones@redhat.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/acpi.c        |  2 -
 arch/x86/kernel/acpi/boot.c     |  2 -
 drivers/acpi/bgrt.c             |  6 --
 drivers/firmware/efi/efi-bgrt.c | 80 ++++++++++++++++++--
 drivers/firmware/efi/efi.c      | 13 ++++
 include/linux/efi-bgrt.h        |  4 +-
 6 files changed, 89 insertions(+), 18 deletions(-)
Lorenzo Pieralisi - Jan. 22, 2019, 5:32 p.m.
On Tue, Jan 22, 2019 at 04:06:16PM +0100, Ard Biesheuvel wrote:
> The bitmap left in the framebuffer by the firmware is described by an
> ACPI table called "BGRT", which describes the size, pixel format and
> the address of a BMP image in memory. While the BGRT ACPI table is
> guaranteed to reside in a "ACPI reclaim" memory region, which is
> never touched by Linux. The BMP image, however, typically resides
> in EFI Boot Services Memory, which may have been overwritten by the
> time the BGRT discovery routine runs.
> 
> So instead, drop the handling from the ACPI init code, and call the
> BGRT parsing code immediately after going over the EFI configuration
> table array, at which time no memory has been touched yet except for
> the .data/.bss regions covered by the static kernel image.
> 
> Unfortunately, this involves a non-trivial amount of ACPI entry
> point and root table parsing, but we cannot rely on the normal
> ACPI infrastructure yet this early in the boot.
> 
> Also note that we cannot take the 'acpi_disabled' global variable
> into account, since it may not have assumed the correct value yet
> (on arm64, the default value is '1' which is overridden to '0' if
> no DT description has been made available by the firmware)
> 
> Cc: Peter Jones <pjones@redhat.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Technically this is a fix, right ?

> ---
>  arch/arm64/kernel/acpi.c        |  2 -
>  arch/x86/kernel/acpi/boot.c     |  2 -
>  drivers/acpi/bgrt.c             |  6 --
>  drivers/firmware/efi/efi-bgrt.c | 80 ++++++++++++++++++--
>  drivers/firmware/efi/efi.c      | 13 ++++
>  include/linux/efi-bgrt.h        |  4 +-
>  6 files changed, 89 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 44e3c351e1ea..7429a811f76d 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -230,8 +230,6 @@ void __init acpi_boot_table_init(void)
>  			early_init_dt_scan_chosen_stdout();
>  	} else {
>  		acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
> -		if (IS_ENABLED(CONFIG_ACPI_BGRT))
> -			acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
>  	}
>  }
>  
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 2624de16cd7a..2d3535b62752 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -1633,8 +1633,6 @@ int __init acpi_boot_init(void)
>  	acpi_process_madt();
>  
>  	acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
> -	if (IS_ENABLED(CONFIG_ACPI_BGRT))
> -		acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
>  
>  	if (!acpi_noirq)
>  		x86_init.pci.init = pci_acpi_init;
> diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c
> index 75af78361ce5..048413e06898 100644
> --- a/drivers/acpi/bgrt.c
> +++ b/drivers/acpi/bgrt.c
> @@ -81,12 +81,6 @@ static const struct attribute_group bgrt_attribute_group = {
>  	.bin_attrs = bgrt_bin_attributes,
>  };
>  
> -int __init acpi_parse_bgrt(struct acpi_table_header *table)
> -{
> -	efi_bgrt_init(table);
> -	return 0;
> -}
> -
>  static int __init bgrt_init(void)
>  {
>  	int ret;
> diff --git a/drivers/firmware/efi/efi-bgrt.c b/drivers/firmware/efi/efi-bgrt.c
> index b22ccfb0c991..8bd9b96942d2 100644
> --- a/drivers/firmware/efi/efi-bgrt.c
> +++ b/drivers/firmware/efi/efi-bgrt.c
> @@ -27,24 +27,92 @@ struct bmp_header {
>  	u32 size;
>  } __packed;
>  
> -void __init efi_bgrt_init(struct acpi_table_header *table)
> +void __init efi_bgrt_init(unsigned long rsdp_phys)
>  {
>  	void *image;
>  	struct bmp_header bmp_header;
>  	struct acpi_table_bgrt *bgrt = &bgrt_tab;
> +	struct acpi_table_bgrt *table = NULL;
> +	struct acpi_table_rsdp *rsdp;
> +	struct acpi_table_header *hdr;
> +	u64 xsdt_phys;
> +	u32 rsdt_phys;
> +	unsigned int len;
>  
> -	if (acpi_disabled)
> +	if (!efi_enabled(EFI_MEMMAP))
>  		return;
>  
> -	if (!efi_enabled(EFI_MEMMAP))
> +	/* map the root pointer table to find the xsdt/rsdt values */
> +	rsdp = early_memremap(rsdp_phys, sizeof(*rsdp));
> +	if (rsdp && ACPI_VALIDATE_RSDP_SIG(rsdp->signature)) {
> +		xsdt_phys = rsdp->xsdt_physical_address;
> +		rsdt_phys = rsdp->rsdt_physical_address;
> +	} else {
> +		WARN_ON(1);

You may need to unmap rsdp.

> +		return;
> +	}
> +	early_memunmap(rsdp, sizeof(*rsdp));
> +
> +	/* obtain the length of whichever table we will be using */
> +	hdr = early_memremap(xsdt_phys ?: rsdt_phys, sizeof(*hdr));
> +	if (WARN_ON(!hdr))
> +		return;
> +	len = hdr->length;
> +	early_memunmap(hdr, sizeof(*hdr));
> +
> +	if (xsdt_phys) {
> +		struct acpi_table_xsdt *xsdt = early_memremap(xsdt_phys, len);
> +		int i;
> +
> +		if (WARN_ON(!xsdt))
> +			return;
> +
> +		for (i = 0; i < (len - sizeof(*hdr)) / sizeof(u64); i++) {
> +			table = early_memremap(xsdt->table_offset_entry[i],
> +					       sizeof(*table));
> +			if (WARN_ON(!table))
> +				break;
> +
> +			if (ACPI_COMPARE_NAME(table->header.signature,
> +					      ACPI_SIG_BGRT))
> +				break;
> +			early_memunmap(table, sizeof(*table));
> +			table = NULL;
> +		}
> +		early_memunmap(xsdt, len);
> +	} else if (rsdt_phys) {
> +		struct acpi_table_rsdt *rsdt = early_memremap(rsdt_phys, len);
> +		int i;
> +
> +		if (WARN_ON(!rsdt))
> +			return;
> +
> +		for (i = 0; i < (len - sizeof(*hdr)) / sizeof(u32); i++) {
> +			table = early_memremap(rsdt->table_offset_entry[i],
> +					       sizeof(*table));
> +			if (WARN_ON(!table))
> +				break;
> +
> +			if (ACPI_COMPARE_NAME(table->header.signature,
> +					      ACPI_SIG_BGRT))
> +				break;
> +			early_memunmap(table, sizeof(*table));
> +			table = NULL;
> +		}
> +		early_memunmap(rsdt, len);
> +	}
> +
> +	if (!table)
>  		return;
>  
> -	if (table->length < sizeof(bgrt_tab)) {
> +	if (table->header.length < sizeof(bgrt_tab)) {
>  		pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
> -		       table->length, sizeof(bgrt_tab));
> +		       table->header.length, sizeof(bgrt_tab));
>  		return;

Likewise for the table pointer.

>  	}
> -	*bgrt = *(struct acpi_table_bgrt *)table;
> +	*bgrt = *table;
> +	early_memunmap(table, sizeof(*table));
> +
>  	if (bgrt->version != 1) {
>  		pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
>  		       bgrt->version);

Side note (not related to this patch):

I do not understand this check:

if (bgrt->status & 0xfe) {
	pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
	       bgrt->status);
	goto out;
}

Should not we check against 0xf8 (reserved bits are [7:3]) ? And what
about status[0] (valid), should we check that as well ?

Thanks,
Lorenzo

> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 4c46ff6f2242..e5ef5c0eacc1 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -20,6 +20,7 @@
>  #include <linux/init.h>
>  #include <linux/device.h>
>  #include <linux/efi.h>
> +#include <linux/efi-bgrt.h>
>  #include <linux/of.h>
>  #include <linux/of_fdt.h>
>  #include <linux/io.h>
> @@ -592,6 +593,18 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
>  
>  		early_memunmap(tbl, sizeof(*tbl));
>  	}
> +
> +	/*
> +	 * We need to parse the BGRT table (which is an ACPI table not a UEFI
> +	 * configuration table) by hand and figure out where the bitmap it
> +	 * describes lives in memory so we can reserve it early on. Otherwise,
> +	 * it may be clobbered by the time we get to it during the ordinary ACPI
> +	 * table init sequence.
> +	 */
> +	if (IS_ENABLED(CONFIG_ACPI_BGRT) &&
> +	    efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> +		efi_bgrt_init(efi.acpi20);
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/efi-bgrt.h b/include/linux/efi-bgrt.h
> index e6cd51005633..528ea62d99ec 100644
> --- a/include/linux/efi-bgrt.h
> +++ b/include/linux/efi-bgrt.h
> @@ -6,7 +6,7 @@
>  
>  #ifdef CONFIG_ACPI_BGRT
>  
> -void efi_bgrt_init(struct acpi_table_header *table);
> +void efi_bgrt_init(unsigned long rsdp_phys);
>  int __init acpi_parse_bgrt(struct acpi_table_header *table);
>  
>  /* The BGRT data itself; only valid if bgrt_image != NULL. */
> @@ -15,7 +15,7 @@ extern struct acpi_table_bgrt bgrt_tab;
>  
>  #else /* !CONFIG_ACPI_BGRT */
>  
> -static inline void efi_bgrt_init(struct acpi_table_header *table) {}
> +static inline void efi_bgrt_init(unsigned long rsdp_phys) {}
>  static inline int __init acpi_parse_bgrt(struct acpi_table_header *table)
>  {
>  	return 0;
> -- 
> 2.20.1
>
Ard Biesheuvel - Jan. 22, 2019, 6:07 p.m.
On Tue, 22 Jan 2019 at 18:32, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Tue, Jan 22, 2019 at 04:06:16PM +0100, Ard Biesheuvel wrote:
> > The bitmap left in the framebuffer by the firmware is described by an
> > ACPI table called "BGRT", which describes the size, pixel format and
> > the address of a BMP image in memory. While the BGRT ACPI table is
> > guaranteed to reside in a "ACPI reclaim" memory region, which is
> > never touched by Linux. The BMP image, however, typically resides
> > in EFI Boot Services Memory, which may have been overwritten by the
> > time the BGRT discovery routine runs.
> >
> > So instead, drop the handling from the ACPI init code, and call the
> > BGRT parsing code immediately after going over the EFI configuration
> > table array, at which time no memory has been touched yet except for
> > the .data/.bss regions covered by the static kernel image.
> >
> > Unfortunately, this involves a non-trivial amount of ACPI entry
> > point and root table parsing, but we cannot rely on the normal
> > ACPI infrastructure yet this early in the boot.
> >
> > Also note that we cannot take the 'acpi_disabled' global variable
> > into account, since it may not have assumed the correct value yet
> > (on arm64, the default value is '1' which is overridden to '0' if
> > no DT description has been made available by the firmware)
> >
> > Cc: Peter Jones <pjones@redhat.com>
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Technically this is a fix, right ?
>

I would argue that it is a hack to work around a deficiency in the
ACPI spec, i.e., that the BGRT table but not the image are passed in
ACPI reclaim memory.

> > ---
> >  arch/arm64/kernel/acpi.c        |  2 -
> >  arch/x86/kernel/acpi/boot.c     |  2 -
> >  drivers/acpi/bgrt.c             |  6 --
> >  drivers/firmware/efi/efi-bgrt.c | 80 ++++++++++++++++++--
> >  drivers/firmware/efi/efi.c      | 13 ++++
> >  include/linux/efi-bgrt.h        |  4 +-
> >  6 files changed, 89 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > index 44e3c351e1ea..7429a811f76d 100644
> > --- a/arch/arm64/kernel/acpi.c
> > +++ b/arch/arm64/kernel/acpi.c
> > @@ -230,8 +230,6 @@ void __init acpi_boot_table_init(void)
> >                       early_init_dt_scan_chosen_stdout();
> >       } else {
> >               acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
> > -             if (IS_ENABLED(CONFIG_ACPI_BGRT))
> > -                     acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
> >       }
> >  }
> >
> > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> > index 2624de16cd7a..2d3535b62752 100644
> > --- a/arch/x86/kernel/acpi/boot.c
> > +++ b/arch/x86/kernel/acpi/boot.c
> > @@ -1633,8 +1633,6 @@ int __init acpi_boot_init(void)
> >       acpi_process_madt();
> >
> >       acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
> > -     if (IS_ENABLED(CONFIG_ACPI_BGRT))
> > -             acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
> >
> >       if (!acpi_noirq)
> >               x86_init.pci.init = pci_acpi_init;
> > diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c
> > index 75af78361ce5..048413e06898 100644
> > --- a/drivers/acpi/bgrt.c
> > +++ b/drivers/acpi/bgrt.c
> > @@ -81,12 +81,6 @@ static const struct attribute_group bgrt_attribute_group = {
> >       .bin_attrs = bgrt_bin_attributes,
> >  };
> >
> > -int __init acpi_parse_bgrt(struct acpi_table_header *table)
> > -{
> > -     efi_bgrt_init(table);
> > -     return 0;
> > -}
> > -
> >  static int __init bgrt_init(void)
> >  {
> >       int ret;
> > diff --git a/drivers/firmware/efi/efi-bgrt.c b/drivers/firmware/efi/efi-bgrt.c
> > index b22ccfb0c991..8bd9b96942d2 100644
> > --- a/drivers/firmware/efi/efi-bgrt.c
> > +++ b/drivers/firmware/efi/efi-bgrt.c
> > @@ -27,24 +27,92 @@ struct bmp_header {
> >       u32 size;
> >  } __packed;
> >
> > -void __init efi_bgrt_init(struct acpi_table_header *table)
> > +void __init efi_bgrt_init(unsigned long rsdp_phys)
> >  {
> >       void *image;
> >       struct bmp_header bmp_header;
> >       struct acpi_table_bgrt *bgrt = &bgrt_tab;
> > +     struct acpi_table_bgrt *table = NULL;
> > +     struct acpi_table_rsdp *rsdp;
> > +     struct acpi_table_header *hdr;
> > +     u64 xsdt_phys;
> > +     u32 rsdt_phys;
> > +     unsigned int len;
> >
> > -     if (acpi_disabled)
> > +     if (!efi_enabled(EFI_MEMMAP))
> >               return;
> >
> > -     if (!efi_enabled(EFI_MEMMAP))
> > +     /* map the root pointer table to find the xsdt/rsdt values */
> > +     rsdp = early_memremap(rsdp_phys, sizeof(*rsdp));
> > +     if (rsdp && ACPI_VALIDATE_RSDP_SIG(rsdp->signature)) {
> > +             xsdt_phys = rsdp->xsdt_physical_address;
> > +             rsdt_phys = rsdp->rsdt_physical_address;
> > +     } else {
> > +             WARN_ON(1);
>
> You may need to unmap rsdp.
>

Indeed.

> > +             return;
> > +     }
> > +     early_memunmap(rsdp, sizeof(*rsdp));
> > +
> > +     /* obtain the length of whichever table we will be using */
> > +     hdr = early_memremap(xsdt_phys ?: rsdt_phys, sizeof(*hdr));
> > +     if (WARN_ON(!hdr))
> > +             return;
> > +     len = hdr->length;
> > +     early_memunmap(hdr, sizeof(*hdr));
> > +
> > +     if (xsdt_phys) {
> > +             struct acpi_table_xsdt *xsdt = early_memremap(xsdt_phys, len);
> > +             int i;
> > +
> > +             if (WARN_ON(!xsdt))
> > +                     return;
> > +
> > +             for (i = 0; i < (len - sizeof(*hdr)) / sizeof(u64); i++) {
> > +                     table = early_memremap(xsdt->table_offset_entry[i],
> > +                                            sizeof(*table));
> > +                     if (WARN_ON(!table))
> > +                             break;
> > +
> > +                     if (ACPI_COMPARE_NAME(table->header.signature,
> > +                                           ACPI_SIG_BGRT))
> > +                             break;
> > +                     early_memunmap(table, sizeof(*table));
> > +                     table = NULL;
> > +             }
> > +             early_memunmap(xsdt, len);
> > +     } else if (rsdt_phys) {
> > +             struct acpi_table_rsdt *rsdt = early_memremap(rsdt_phys, len);
> > +             int i;
> > +
> > +             if (WARN_ON(!rsdt))
> > +                     return;
> > +
> > +             for (i = 0; i < (len - sizeof(*hdr)) / sizeof(u32); i++) {
> > +                     table = early_memremap(rsdt->table_offset_entry[i],
> > +                                            sizeof(*table));
> > +                     if (WARN_ON(!table))
> > +                             break;
> > +
> > +                     if (ACPI_COMPARE_NAME(table->header.signature,
> > +                                           ACPI_SIG_BGRT))
> > +                             break;
> > +                     early_memunmap(table, sizeof(*table));
> > +                     table = NULL;
> > +             }
> > +             early_memunmap(rsdt, len);
> > +     }
> > +
> > +     if (!table)
> >               return;
> >
> > -     if (table->length < sizeof(bgrt_tab)) {
> > +     if (table->header.length < sizeof(bgrt_tab)) {
> >               pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
> > -                    table->length, sizeof(bgrt_tab));
> > +                    table->header.length, sizeof(bgrt_tab));
> >               return;
>
> Likewise for the table pointer.
>

Yep.

> >       }
> > -     *bgrt = *(struct acpi_table_bgrt *)table;
> > +     *bgrt = *table;
> > +     early_memunmap(table, sizeof(*table));
> > +
> >       if (bgrt->version != 1) {
> >               pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
> >                      bgrt->version);
>
> Side note (not related to this patch):
>
> I do not understand this check:
>
> if (bgrt->status & 0xfe) {
>         pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
>                bgrt->status);
>         goto out;
> }
>
> Should not we check against 0xf8 (reserved bits are [7:3]) ? And what
> about status[0] (valid), should we check that as well ?
>

I will let Peter take that question ...

Thanks,
Ard.

> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 4c46ff6f2242..e5ef5c0eacc1 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/init.h>
> >  #include <linux/device.h>
> >  #include <linux/efi.h>
> > +#include <linux/efi-bgrt.h>
> >  #include <linux/of.h>
> >  #include <linux/of_fdt.h>
> >  #include <linux/io.h>
> > @@ -592,6 +593,18 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
> >
> >               early_memunmap(tbl, sizeof(*tbl));
> >       }
> > +
> > +     /*
> > +      * We need to parse the BGRT table (which is an ACPI table not a UEFI
> > +      * configuration table) by hand and figure out where the bitmap it
> > +      * describes lives in memory so we can reserve it early on. Otherwise,
> > +      * it may be clobbered by the time we get to it during the ordinary ACPI
> > +      * table init sequence.
> > +      */
> > +     if (IS_ENABLED(CONFIG_ACPI_BGRT) &&
> > +         efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> > +             efi_bgrt_init(efi.acpi20);
> > +
> >       return 0;
> >  }
> >
> > diff --git a/include/linux/efi-bgrt.h b/include/linux/efi-bgrt.h
> > index e6cd51005633..528ea62d99ec 100644
> > --- a/include/linux/efi-bgrt.h
> > +++ b/include/linux/efi-bgrt.h
> > @@ -6,7 +6,7 @@
> >
> >  #ifdef CONFIG_ACPI_BGRT
> >
> > -void efi_bgrt_init(struct acpi_table_header *table);
> > +void efi_bgrt_init(unsigned long rsdp_phys);
> >  int __init acpi_parse_bgrt(struct acpi_table_header *table);
> >
> >  /* The BGRT data itself; only valid if bgrt_image != NULL. */
> > @@ -15,7 +15,7 @@ extern struct acpi_table_bgrt bgrt_tab;
> >
> >  #else /* !CONFIG_ACPI_BGRT */
> >
> > -static inline void efi_bgrt_init(struct acpi_table_header *table) {}
> > +static inline void efi_bgrt_init(unsigned long rsdp_phys) {}
> >  static inline int __init acpi_parse_bgrt(struct acpi_table_header *table)
> >  {
> >       return 0;
> > --
> > 2.20.1
> >
Lorenzo Pieralisi - Jan. 22, 2019, 6:56 p.m.
On Tue, Jan 22, 2019 at 07:07:30PM +0100, Ard Biesheuvel wrote:
> On Tue, 22 Jan 2019 at 18:32, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > On Tue, Jan 22, 2019 at 04:06:16PM +0100, Ard Biesheuvel wrote:
> > > The bitmap left in the framebuffer by the firmware is described by an
> > > ACPI table called "BGRT", which describes the size, pixel format and
> > > the address of a BMP image in memory. While the BGRT ACPI table is
> > > guaranteed to reside in a "ACPI reclaim" memory region, which is
> > > never touched by Linux. The BMP image, however, typically resides
> > > in EFI Boot Services Memory, which may have been overwritten by the
> > > time the BGRT discovery routine runs.
> > >
> > > So instead, drop the handling from the ACPI init code, and call the
> > > BGRT parsing code immediately after going over the EFI configuration
> > > table array, at which time no memory has been touched yet except for
> > > the .data/.bss regions covered by the static kernel image.
> > >
> > > Unfortunately, this involves a non-trivial amount of ACPI entry
> > > point and root table parsing, but we cannot rely on the normal
> > > ACPI infrastructure yet this early in the boot.
> > >
> > > Also note that we cannot take the 'acpi_disabled' global variable
> > > into account, since it may not have assumed the correct value yet
> > > (on arm64, the default value is '1' which is overridden to '0' if
> > > no DT description has been made available by the firmware)
> > >
> > > Cc: Peter Jones <pjones@redhat.com>
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > Technically this is a fix, right ?
> >
> 
> I would argue that it is a hack to work around a deficiency in the
> ACPI spec, i.e., that the BGRT table but not the image are passed in
> ACPI reclaim memory.

I assume there are designs out there that this patch is fixing (I
understand it is hackish but there is not much else you can do,
especially so with firmware already in the field), I was wondering
if we want to update older kernels too.

Cheers,
Lorenzo
Dave Young - Jan. 23, 2019, 5:31 a.m.
Hi,

On 01/22/19 at 04:06pm, Ard Biesheuvel wrote:
> The bitmap left in the framebuffer by the firmware is described by an
> ACPI table called "BGRT", which describes the size, pixel format and
> the address of a BMP image in memory. While the BGRT ACPI table is
> guaranteed to reside in a "ACPI reclaim" memory region, which is
> never touched by Linux. The BMP image, however, typically resides
> in EFI Boot Services Memory, which may have been overwritten by the
> time the BGRT discovery routine runs.

I vaguely remember boot service memory is reserved on X86,  so this
is an issue of arm only?

> 
> So instead, drop the handling from the ACPI init code, and call the
> BGRT parsing code immediately after going over the EFI configuration
> table array, at which time no memory has been touched yet except for
> the .data/.bss regions covered by the static kernel image.
> 
> Unfortunately, this involves a non-trivial amount of ACPI entry
> point and root table parsing, but we cannot rely on the normal
> ACPI infrastructure yet this early in the boot.
> 
> Also note that we cannot take the 'acpi_disabled' global variable
> into account, since it may not have assumed the correct value yet
> (on arm64, the default value is '1' which is overridden to '0' if
> no DT description has been made available by the firmware)
> 
> Cc: Peter Jones <pjones@redhat.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/acpi.c        |  2 -
>  arch/x86/kernel/acpi/boot.c     |  2 -
>  drivers/acpi/bgrt.c             |  6 --
>  drivers/firmware/efi/efi-bgrt.c | 80 ++++++++++++++++++--
>  drivers/firmware/efi/efi.c      | 13 ++++
>  include/linux/efi-bgrt.h        |  4 +-
>  6 files changed, 89 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 44e3c351e1ea..7429a811f76d 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -230,8 +230,6 @@ void __init acpi_boot_table_init(void)
>  			early_init_dt_scan_chosen_stdout();
>  	} else {
>  		acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
> -		if (IS_ENABLED(CONFIG_ACPI_BGRT))
> -			acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
>  	}
>  }
>  
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 2624de16cd7a..2d3535b62752 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -1633,8 +1633,6 @@ int __init acpi_boot_init(void)
>  	acpi_process_madt();
>  
>  	acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
> -	if (IS_ENABLED(CONFIG_ACPI_BGRT))
> -		acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
>  
>  	if (!acpi_noirq)
>  		x86_init.pci.init = pci_acpi_init;
> diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c
> index 75af78361ce5..048413e06898 100644
> --- a/drivers/acpi/bgrt.c
> +++ b/drivers/acpi/bgrt.c
> @@ -81,12 +81,6 @@ static const struct attribute_group bgrt_attribute_group = {
>  	.bin_attrs = bgrt_bin_attributes,
>  };
>  
> -int __init acpi_parse_bgrt(struct acpi_table_header *table)
> -{
> -	efi_bgrt_init(table);
> -	return 0;
> -}
> -
>  static int __init bgrt_init(void)
>  {
>  	int ret;
> diff --git a/drivers/firmware/efi/efi-bgrt.c b/drivers/firmware/efi/efi-bgrt.c
> index b22ccfb0c991..8bd9b96942d2 100644
> --- a/drivers/firmware/efi/efi-bgrt.c
> +++ b/drivers/firmware/efi/efi-bgrt.c
> @@ -27,24 +27,92 @@ struct bmp_header {
>  	u32 size;
>  } __packed;
>  
> -void __init efi_bgrt_init(struct acpi_table_header *table)
> +void __init efi_bgrt_init(unsigned long rsdp_phys)
>  {
>  	void *image;
>  	struct bmp_header bmp_header;
>  	struct acpi_table_bgrt *bgrt = &bgrt_tab;
> +	struct acpi_table_bgrt *table = NULL;
> +	struct acpi_table_rsdp *rsdp;
> +	struct acpi_table_header *hdr;
> +	u64 xsdt_phys;
> +	u32 rsdt_phys;
> +	unsigned int len;
>  
> -	if (acpi_disabled)
> +	if (!efi_enabled(EFI_MEMMAP))
>  		return;
>  
> -	if (!efi_enabled(EFI_MEMMAP))
> +	/* map the root pointer table to find the xsdt/rsdt values */
> +	rsdp = early_memremap(rsdp_phys, sizeof(*rsdp));
> +	if (rsdp && ACPI_VALIDATE_RSDP_SIG(rsdp->signature)) {
> +		xsdt_phys = rsdp->xsdt_physical_address;
> +		rsdt_phys = rsdp->rsdt_physical_address;
> +	} else {
> +		WARN_ON(1);
> +		return;
> +	}
> +	early_memunmap(rsdp, sizeof(*rsdp));
> +
> +	/* obtain the length of whichever table we will be using */
> +	hdr = early_memremap(xsdt_phys ?: rsdt_phys, sizeof(*hdr));
> +	if (WARN_ON(!hdr))
> +		return;
> +	len = hdr->length;
> +	early_memunmap(hdr, sizeof(*hdr));
> +
> +	if (xsdt_phys) {
> +		struct acpi_table_xsdt *xsdt = early_memremap(xsdt_phys, len);
> +		int i;
> +
> +		if (WARN_ON(!xsdt))
> +			return;
> +
> +		for (i = 0; i < (len - sizeof(*hdr)) / sizeof(u64); i++) {
> +			table = early_memremap(xsdt->table_offset_entry[i],
> +					       sizeof(*table));
> +			if (WARN_ON(!table))
> +				break;
> +
> +			if (ACPI_COMPARE_NAME(table->header.signature,
> +					      ACPI_SIG_BGRT))
> +				break;
> +			early_memunmap(table, sizeof(*table));
> +			table = NULL;
> +		}
> +		early_memunmap(xsdt, len);
> +	} else if (rsdt_phys) {
> +		struct acpi_table_rsdt *rsdt = early_memremap(rsdt_phys, len);
> +		int i;
> +
> +		if (WARN_ON(!rsdt))
> +			return;
> +
> +		for (i = 0; i < (len - sizeof(*hdr)) / sizeof(u32); i++) {
> +			table = early_memremap(rsdt->table_offset_entry[i],
> +					       sizeof(*table));
> +			if (WARN_ON(!table))
> +				break;
> +
> +			if (ACPI_COMPARE_NAME(table->header.signature,
> +					      ACPI_SIG_BGRT))
> +				break;
> +			early_memunmap(table, sizeof(*table));
> +			table = NULL;
> +		}
> +		early_memunmap(rsdt, len);
> +	}
> +
> +	if (!table)
>  		return;
>  
> -	if (table->length < sizeof(bgrt_tab)) {
> +	if (table->header.length < sizeof(bgrt_tab)) {
>  		pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
> -		       table->length, sizeof(bgrt_tab));
> +		       table->header.length, sizeof(bgrt_tab));
>  		return;
>  	}
> -	*bgrt = *(struct acpi_table_bgrt *)table;
> +	*bgrt = *table;
> +	early_memunmap(table, sizeof(*table));
> +
>  	if (bgrt->version != 1) {
>  		pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
>  		       bgrt->version);
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 4c46ff6f2242..e5ef5c0eacc1 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -20,6 +20,7 @@
>  #include <linux/init.h>
>  #include <linux/device.h>
>  #include <linux/efi.h>
> +#include <linux/efi-bgrt.h>
>  #include <linux/of.h>
>  #include <linux/of_fdt.h>
>  #include <linux/io.h>
> @@ -592,6 +593,18 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
>  
>  		early_memunmap(tbl, sizeof(*tbl));
>  	}
> +
> +	/*
> +	 * We need to parse the BGRT table (which is an ACPI table not a UEFI
> +	 * configuration table) by hand and figure out where the bitmap it
> +	 * describes lives in memory so we can reserve it early on. Otherwise,
> +	 * it may be clobbered by the time we get to it during the ordinary ACPI
> +	 * table init sequence.
> +	 */
> +	if (IS_ENABLED(CONFIG_ACPI_BGRT) &&
> +	    efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> +		efi_bgrt_init(efi.acpi20);
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/efi-bgrt.h b/include/linux/efi-bgrt.h
> index e6cd51005633..528ea62d99ec 100644
> --- a/include/linux/efi-bgrt.h
> +++ b/include/linux/efi-bgrt.h
> @@ -6,7 +6,7 @@
>  
>  #ifdef CONFIG_ACPI_BGRT
>  
> -void efi_bgrt_init(struct acpi_table_header *table);
> +void efi_bgrt_init(unsigned long rsdp_phys);
>  int __init acpi_parse_bgrt(struct acpi_table_header *table);
>  
>  /* The BGRT data itself; only valid if bgrt_image != NULL. */
> @@ -15,7 +15,7 @@ extern struct acpi_table_bgrt bgrt_tab;
>  
>  #else /* !CONFIG_ACPI_BGRT */
>  
> -static inline void efi_bgrt_init(struct acpi_table_header *table) {}
> +static inline void efi_bgrt_init(unsigned long rsdp_phys) {}
>  static inline int __init acpi_parse_bgrt(struct acpi_table_header *table)
>  {
>  	return 0;
> -- 
> 2.20.1
> 

Thanks
Dave
Ard Biesheuvel - Jan. 23, 2019, 9:44 a.m.
On Wed, 23 Jan 2019 at 06:31, Dave Young <dyoung@redhat.com> wrote:
>
> Hi,
>
> On 01/22/19 at 04:06pm, Ard Biesheuvel wrote:
> > The bitmap left in the framebuffer by the firmware is described by an
> > ACPI table called "BGRT", which describes the size, pixel format and
> > the address of a BMP image in memory. While the BGRT ACPI table is
> > guaranteed to reside in a "ACPI reclaim" memory region, which is
> > never touched by Linux. The BMP image, however, typically resides
> > in EFI Boot Services Memory, which may have been overwritten by the
> > time the BGRT discovery routine runs.
>
> I vaguely remember boot service memory is reserved on X86,  so this
> is an issue of arm only?
>

No both ARM and x86 are affected. Harry and/or Peter should be able to
comment on the details.

> >
> > So instead, drop the handling from the ACPI init code, and call the
> > BGRT parsing code immediately after going over the EFI configuration
> > table array, at which time no memory has been touched yet except for
> > the .data/.bss regions covered by the static kernel image.
> >
> > Unfortunately, this involves a non-trivial amount of ACPI entry
> > point and root table parsing, but we cannot rely on the normal
> > ACPI infrastructure yet this early in the boot.
> >
> > Also note that we cannot take the 'acpi_disabled' global variable
> > into account, since it may not have assumed the correct value yet
> > (on arm64, the default value is '1' which is overridden to '0' if
> > no DT description has been made available by the firmware)
> >
> > Cc: Peter Jones <pjones@redhat.com>
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  arch/arm64/kernel/acpi.c        |  2 -
> >  arch/x86/kernel/acpi/boot.c     |  2 -
> >  drivers/acpi/bgrt.c             |  6 --
> >  drivers/firmware/efi/efi-bgrt.c | 80 ++++++++++++++++++--
> >  drivers/firmware/efi/efi.c      | 13 ++++
> >  include/linux/efi-bgrt.h        |  4 +-
> >  6 files changed, 89 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > index 44e3c351e1ea..7429a811f76d 100644
> > --- a/arch/arm64/kernel/acpi.c
> > +++ b/arch/arm64/kernel/acpi.c
> > @@ -230,8 +230,6 @@ void __init acpi_boot_table_init(void)
> >                       early_init_dt_scan_chosen_stdout();
> >       } else {
> >               acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
> > -             if (IS_ENABLED(CONFIG_ACPI_BGRT))
> > -                     acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
> >       }
> >  }
> >
> > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> > index 2624de16cd7a..2d3535b62752 100644
> > --- a/arch/x86/kernel/acpi/boot.c
> > +++ b/arch/x86/kernel/acpi/boot.c
> > @@ -1633,8 +1633,6 @@ int __init acpi_boot_init(void)
> >       acpi_process_madt();
> >
> >       acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
> > -     if (IS_ENABLED(CONFIG_ACPI_BGRT))
> > -             acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
> >
> >       if (!acpi_noirq)
> >               x86_init.pci.init = pci_acpi_init;
> > diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c
> > index 75af78361ce5..048413e06898 100644
> > --- a/drivers/acpi/bgrt.c
> > +++ b/drivers/acpi/bgrt.c
> > @@ -81,12 +81,6 @@ static const struct attribute_group bgrt_attribute_group = {
> >       .bin_attrs = bgrt_bin_attributes,
> >  };
> >
> > -int __init acpi_parse_bgrt(struct acpi_table_header *table)
> > -{
> > -     efi_bgrt_init(table);
> > -     return 0;
> > -}
> > -
> >  static int __init bgrt_init(void)
> >  {
> >       int ret;
> > diff --git a/drivers/firmware/efi/efi-bgrt.c b/drivers/firmware/efi/efi-bgrt.c
> > index b22ccfb0c991..8bd9b96942d2 100644
> > --- a/drivers/firmware/efi/efi-bgrt.c
> > +++ b/drivers/firmware/efi/efi-bgrt.c
> > @@ -27,24 +27,92 @@ struct bmp_header {
> >       u32 size;
> >  } __packed;
> >
> > -void __init efi_bgrt_init(struct acpi_table_header *table)
> > +void __init efi_bgrt_init(unsigned long rsdp_phys)
> >  {
> >       void *image;
> >       struct bmp_header bmp_header;
> >       struct acpi_table_bgrt *bgrt = &bgrt_tab;
> > +     struct acpi_table_bgrt *table = NULL;
> > +     struct acpi_table_rsdp *rsdp;
> > +     struct acpi_table_header *hdr;
> > +     u64 xsdt_phys;
> > +     u32 rsdt_phys;
> > +     unsigned int len;
> >
> > -     if (acpi_disabled)
> > +     if (!efi_enabled(EFI_MEMMAP))
> >               return;
> >
> > -     if (!efi_enabled(EFI_MEMMAP))
> > +     /* map the root pointer table to find the xsdt/rsdt values */
> > +     rsdp = early_memremap(rsdp_phys, sizeof(*rsdp));
> > +     if (rsdp && ACPI_VALIDATE_RSDP_SIG(rsdp->signature)) {
> > +             xsdt_phys = rsdp->xsdt_physical_address;
> > +             rsdt_phys = rsdp->rsdt_physical_address;
> > +     } else {
> > +             WARN_ON(1);
> > +             return;
> > +     }
> > +     early_memunmap(rsdp, sizeof(*rsdp));
> > +
> > +     /* obtain the length of whichever table we will be using */
> > +     hdr = early_memremap(xsdt_phys ?: rsdt_phys, sizeof(*hdr));
> > +     if (WARN_ON(!hdr))
> > +             return;
> > +     len = hdr->length;
> > +     early_memunmap(hdr, sizeof(*hdr));
> > +
> > +     if (xsdt_phys) {
> > +             struct acpi_table_xsdt *xsdt = early_memremap(xsdt_phys, len);
> > +             int i;
> > +
> > +             if (WARN_ON(!xsdt))
> > +                     return;
> > +
> > +             for (i = 0; i < (len - sizeof(*hdr)) / sizeof(u64); i++) {
> > +                     table = early_memremap(xsdt->table_offset_entry[i],
> > +                                            sizeof(*table));
> > +                     if (WARN_ON(!table))
> > +                             break;
> > +
> > +                     if (ACPI_COMPARE_NAME(table->header.signature,
> > +                                           ACPI_SIG_BGRT))
> > +                             break;
> > +                     early_memunmap(table, sizeof(*table));
> > +                     table = NULL;
> > +             }
> > +             early_memunmap(xsdt, len);
> > +     } else if (rsdt_phys) {
> > +             struct acpi_table_rsdt *rsdt = early_memremap(rsdt_phys, len);
> > +             int i;
> > +
> > +             if (WARN_ON(!rsdt))
> > +                     return;
> > +
> > +             for (i = 0; i < (len - sizeof(*hdr)) / sizeof(u32); i++) {
> > +                     table = early_memremap(rsdt->table_offset_entry[i],
> > +                                            sizeof(*table));
> > +                     if (WARN_ON(!table))
> > +                             break;
> > +
> > +                     if (ACPI_COMPARE_NAME(table->header.signature,
> > +                                           ACPI_SIG_BGRT))
> > +                             break;
> > +                     early_memunmap(table, sizeof(*table));
> > +                     table = NULL;
> > +             }
> > +             early_memunmap(rsdt, len);
> > +     }
> > +
> > +     if (!table)
> >               return;
> >
> > -     if (table->length < sizeof(bgrt_tab)) {
> > +     if (table->header.length < sizeof(bgrt_tab)) {
> >               pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
> > -                    table->length, sizeof(bgrt_tab));
> > +                    table->header.length, sizeof(bgrt_tab));
> >               return;
> >       }
> > -     *bgrt = *(struct acpi_table_bgrt *)table;
> > +     *bgrt = *table;
> > +     early_memunmap(table, sizeof(*table));
> > +
> >       if (bgrt->version != 1) {
> >               pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
> >                      bgrt->version);
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 4c46ff6f2242..e5ef5c0eacc1 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/init.h>
> >  #include <linux/device.h>
> >  #include <linux/efi.h>
> > +#include <linux/efi-bgrt.h>
> >  #include <linux/of.h>
> >  #include <linux/of_fdt.h>
> >  #include <linux/io.h>
> > @@ -592,6 +593,18 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
> >
> >               early_memunmap(tbl, sizeof(*tbl));
> >       }
> > +
> > +     /*
> > +      * We need to parse the BGRT table (which is an ACPI table not a UEFI
> > +      * configuration table) by hand and figure out where the bitmap it
> > +      * describes lives in memory so we can reserve it early on. Otherwise,
> > +      * it may be clobbered by the time we get to it during the ordinary ACPI
> > +      * table init sequence.
> > +      */
> > +     if (IS_ENABLED(CONFIG_ACPI_BGRT) &&
> > +         efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> > +             efi_bgrt_init(efi.acpi20);
> > +
> >       return 0;
> >  }
> >
> > diff --git a/include/linux/efi-bgrt.h b/include/linux/efi-bgrt.h
> > index e6cd51005633..528ea62d99ec 100644
> > --- a/include/linux/efi-bgrt.h
> > +++ b/include/linux/efi-bgrt.h
> > @@ -6,7 +6,7 @@
> >
> >  #ifdef CONFIG_ACPI_BGRT
> >
> > -void efi_bgrt_init(struct acpi_table_header *table);
> > +void efi_bgrt_init(unsigned long rsdp_phys);
> >  int __init acpi_parse_bgrt(struct acpi_table_header *table);
> >
> >  /* The BGRT data itself; only valid if bgrt_image != NULL. */
> > @@ -15,7 +15,7 @@ extern struct acpi_table_bgrt bgrt_tab;
> >
> >  #else /* !CONFIG_ACPI_BGRT */
> >
> > -static inline void efi_bgrt_init(struct acpi_table_header *table) {}
> > +static inline void efi_bgrt_init(unsigned long rsdp_phys) {}
> >  static inline int __init acpi_parse_bgrt(struct acpi_table_header *table)
> >  {
> >       return 0;
> > --
> > 2.20.1
> >
>
> Thanks
> Dave
Ard Biesheuvel - Jan. 29, 2019, 2:36 p.m.
On Tue, 22 Jan 2019 at 16:06, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> The bitmap left in the framebuffer by the firmware is described by an
> ACPI table called "BGRT", which describes the size, pixel format and
> the address of a BMP image in memory. While the BGRT ACPI table is
> guaranteed to reside in a "ACPI reclaim" memory region, which is
> never touched by Linux. The BMP image, however, typically resides
> in EFI Boot Services Memory, which may have been overwritten by the
> time the BGRT discovery routine runs.
>
> So instead, drop the handling from the ACPI init code, and call the
> BGRT parsing code immediately after going over the EFI configuration
> table array, at which time no memory has been touched yet except for
> the .data/.bss regions covered by the static kernel image.
>
> Unfortunately, this involves a non-trivial amount of ACPI entry
> point and root table parsing, but we cannot rely on the normal
> ACPI infrastructure yet this early in the boot.
>
> Also note that we cannot take the 'acpi_disabled' global variable
> into account, since it may not have assumed the correct value yet
> (on arm64, the default value is '1' which is overridden to '0' if
> no DT description has been made available by the firmware)
>
> Cc: Peter Jones <pjones@redhat.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/acpi.c        |  2 -
>  arch/x86/kernel/acpi/boot.c     |  2 -
>  drivers/acpi/bgrt.c             |  6 --
>  drivers/firmware/efi/efi-bgrt.c | 80 ++++++++++++++++++--
>  drivers/firmware/efi/efi.c      | 13 ++++
>  include/linux/efi-bgrt.h        |  4 +-
>  6 files changed, 89 insertions(+), 18 deletions(-)
>

Rafael, Len,

Do you mind if i take this via the EFI tree (after addressing
Lorenzo's comments)

Thanks,
Ard.



> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 44e3c351e1ea..7429a811f76d 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -230,8 +230,6 @@ void __init acpi_boot_table_init(void)
>                         early_init_dt_scan_chosen_stdout();
>         } else {
>                 acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
> -               if (IS_ENABLED(CONFIG_ACPI_BGRT))
> -                       acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
>         }
>  }
>
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 2624de16cd7a..2d3535b62752 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -1633,8 +1633,6 @@ int __init acpi_boot_init(void)
>         acpi_process_madt();
>
>         acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
> -       if (IS_ENABLED(CONFIG_ACPI_BGRT))
> -               acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
>
>         if (!acpi_noirq)
>                 x86_init.pci.init = pci_acpi_init;
> diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c
> index 75af78361ce5..048413e06898 100644
> --- a/drivers/acpi/bgrt.c
> +++ b/drivers/acpi/bgrt.c
> @@ -81,12 +81,6 @@ static const struct attribute_group bgrt_attribute_group = {
>         .bin_attrs = bgrt_bin_attributes,
>  };
>
> -int __init acpi_parse_bgrt(struct acpi_table_header *table)
> -{
> -       efi_bgrt_init(table);
> -       return 0;
> -}
> -
>  static int __init bgrt_init(void)
>  {
>         int ret;
> diff --git a/drivers/firmware/efi/efi-bgrt.c b/drivers/firmware/efi/efi-bgrt.c
> index b22ccfb0c991..8bd9b96942d2 100644
> --- a/drivers/firmware/efi/efi-bgrt.c
> +++ b/drivers/firmware/efi/efi-bgrt.c
> @@ -27,24 +27,92 @@ struct bmp_header {
>         u32 size;
>  } __packed;
>
> -void __init efi_bgrt_init(struct acpi_table_header *table)
> +void __init efi_bgrt_init(unsigned long rsdp_phys)
>  {
>         void *image;
>         struct bmp_header bmp_header;
>         struct acpi_table_bgrt *bgrt = &bgrt_tab;
> +       struct acpi_table_bgrt *table = NULL;
> +       struct acpi_table_rsdp *rsdp;
> +       struct acpi_table_header *hdr;
> +       u64 xsdt_phys;
> +       u32 rsdt_phys;
> +       unsigned int len;
>
> -       if (acpi_disabled)
> +       if (!efi_enabled(EFI_MEMMAP))
>                 return;
>
> -       if (!efi_enabled(EFI_MEMMAP))
> +       /* map the root pointer table to find the xsdt/rsdt values */
> +       rsdp = early_memremap(rsdp_phys, sizeof(*rsdp));
> +       if (rsdp && ACPI_VALIDATE_RSDP_SIG(rsdp->signature)) {
> +               xsdt_phys = rsdp->xsdt_physical_address;
> +               rsdt_phys = rsdp->rsdt_physical_address;
> +       } else {
> +               WARN_ON(1);
> +               return;
> +       }
> +       early_memunmap(rsdp, sizeof(*rsdp));
> +
> +       /* obtain the length of whichever table we will be using */
> +       hdr = early_memremap(xsdt_phys ?: rsdt_phys, sizeof(*hdr));
> +       if (WARN_ON(!hdr))
> +               return;
> +       len = hdr->length;
> +       early_memunmap(hdr, sizeof(*hdr));
> +
> +       if (xsdt_phys) {
> +               struct acpi_table_xsdt *xsdt = early_memremap(xsdt_phys, len);
> +               int i;
> +
> +               if (WARN_ON(!xsdt))
> +                       return;
> +
> +               for (i = 0; i < (len - sizeof(*hdr)) / sizeof(u64); i++) {
> +                       table = early_memremap(xsdt->table_offset_entry[i],
> +                                              sizeof(*table));
> +                       if (WARN_ON(!table))
> +                               break;
> +
> +                       if (ACPI_COMPARE_NAME(table->header.signature,
> +                                             ACPI_SIG_BGRT))
> +                               break;
> +                       early_memunmap(table, sizeof(*table));
> +                       table = NULL;
> +               }
> +               early_memunmap(xsdt, len);
> +       } else if (rsdt_phys) {
> +               struct acpi_table_rsdt *rsdt = early_memremap(rsdt_phys, len);
> +               int i;
> +
> +               if (WARN_ON(!rsdt))
> +                       return;
> +
> +               for (i = 0; i < (len - sizeof(*hdr)) / sizeof(u32); i++) {
> +                       table = early_memremap(rsdt->table_offset_entry[i],
> +                                              sizeof(*table));
> +                       if (WARN_ON(!table))
> +                               break;
> +
> +                       if (ACPI_COMPARE_NAME(table->header.signature,
> +                                             ACPI_SIG_BGRT))
> +                               break;
> +                       early_memunmap(table, sizeof(*table));
> +                       table = NULL;
> +               }
> +               early_memunmap(rsdt, len);
> +       }
> +
> +       if (!table)
>                 return;
>
> -       if (table->length < sizeof(bgrt_tab)) {
> +       if (table->header.length < sizeof(bgrt_tab)) {
>                 pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
> -                      table->length, sizeof(bgrt_tab));
> +                      table->header.length, sizeof(bgrt_tab));
>                 return;
>         }
> -       *bgrt = *(struct acpi_table_bgrt *)table;
> +       *bgrt = *table;
> +       early_memunmap(table, sizeof(*table));
> +
>         if (bgrt->version != 1) {
>                 pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
>                        bgrt->version);
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 4c46ff6f2242..e5ef5c0eacc1 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -20,6 +20,7 @@
>  #include <linux/init.h>
>  #include <linux/device.h>
>  #include <linux/efi.h>
> +#include <linux/efi-bgrt.h>
>  #include <linux/of.h>
>  #include <linux/of_fdt.h>
>  #include <linux/io.h>
> @@ -592,6 +593,18 @@ int __init efi_config_parse_tables(void *config_tables, int count, int sz,
>
>                 early_memunmap(tbl, sizeof(*tbl));
>         }
> +
> +       /*
> +        * We need to parse the BGRT table (which is an ACPI table not a UEFI
> +        * configuration table) by hand and figure out where the bitmap it
> +        * describes lives in memory so we can reserve it early on. Otherwise,
> +        * it may be clobbered by the time we get to it during the ordinary ACPI
> +        * table init sequence.
> +        */
> +       if (IS_ENABLED(CONFIG_ACPI_BGRT) &&
> +           efi.acpi20 != EFI_INVALID_TABLE_ADDR)
> +               efi_bgrt_init(efi.acpi20);
> +
>         return 0;
>  }
>
> diff --git a/include/linux/efi-bgrt.h b/include/linux/efi-bgrt.h
> index e6cd51005633..528ea62d99ec 100644
> --- a/include/linux/efi-bgrt.h
> +++ b/include/linux/efi-bgrt.h
> @@ -6,7 +6,7 @@
>
>  #ifdef CONFIG_ACPI_BGRT
>
> -void efi_bgrt_init(struct acpi_table_header *table);
> +void efi_bgrt_init(unsigned long rsdp_phys);
>  int __init acpi_parse_bgrt(struct acpi_table_header *table);
>
>  /* The BGRT data itself; only valid if bgrt_image != NULL. */
> @@ -15,7 +15,7 @@ extern struct acpi_table_bgrt bgrt_tab;
>
>  #else /* !CONFIG_ACPI_BGRT */
>
> -static inline void efi_bgrt_init(struct acpi_table_header *table) {}
> +static inline void efi_bgrt_init(unsigned long rsdp_phys) {}
>  static inline int __init acpi_parse_bgrt(struct acpi_table_header *table)
>  {
>         return 0;
> --
> 2.20.1
>
Rafael J. Wysocki - Jan. 29, 2019, 3:09 p.m.
On Tue, Jan 29, 2019 at 3:36 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On Tue, 22 Jan 2019 at 16:06, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > The bitmap left in the framebuffer by the firmware is described by an
> > ACPI table called "BGRT", which describes the size, pixel format and
> > the address of a BMP image in memory. While the BGRT ACPI table is
> > guaranteed to reside in a "ACPI reclaim" memory region, which is
> > never touched by Linux. The BMP image, however, typically resides
> > in EFI Boot Services Memory, which may have been overwritten by the
> > time the BGRT discovery routine runs.
> >
> > So instead, drop the handling from the ACPI init code, and call the
> > BGRT parsing code immediately after going over the EFI configuration
> > table array, at which time no memory has been touched yet except for
> > the .data/.bss regions covered by the static kernel image.
> >
> > Unfortunately, this involves a non-trivial amount of ACPI entry
> > point and root table parsing, but we cannot rely on the normal
> > ACPI infrastructure yet this early in the boot.
> >
> > Also note that we cannot take the 'acpi_disabled' global variable
> > into account, since it may not have assumed the correct value yet
> > (on arm64, the default value is '1' which is overridden to '0' if
> > no DT description has been made available by the firmware)
> >
> > Cc: Peter Jones <pjones@redhat.com>
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  arch/arm64/kernel/acpi.c        |  2 -
> >  arch/x86/kernel/acpi/boot.c     |  2 -
> >  drivers/acpi/bgrt.c             |  6 --
> >  drivers/firmware/efi/efi-bgrt.c | 80 ++++++++++++++++++--
> >  drivers/firmware/efi/efi.c      | 13 ++++
> >  include/linux/efi-bgrt.h        |  4 +-
> >  6 files changed, 89 insertions(+), 18 deletions(-)
> >
>
> Rafael, Len,
>
> Do you mind if i take this via the EFI tree (after addressing
> Lorenzo's comments)

Not at all, please take it.

Thanks!
Peter Jones - Jan. 29, 2019, 3:34 p.m.
On Tue, Jan 22, 2019 at 07:07:30PM +0100, Ard Biesheuvel wrote:
> On Tue, 22 Jan 2019 at 18:32, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > Side note (not related to this patch):
> >
> > I do not understand this check:
> >
> > if (bgrt->status & 0xfe) {
> >         pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n",
> >                bgrt->status);
> >         goto out;
> > }
> >
> > Should not we check against 0xf8 (reserved bits are [7:3]) ? And what
> > about status[0] (valid), should we check that as well ?
> 
> I will let Peter take that question ...

I think this was originally coded from a draft spec, but also in ACPI
6.1A bits [2:1] got defined without updating the "version" field :/, so
if we're going to check them, checking against 0xf8 would be correct -
but it's probably saner just to not check them?  Nothing that's been
defined so far is actually a reason not to expose the data, and it's not
clear that anything in the future would either.  Also, the "valid" bit
is meant only to reflect if the image is on the screen or not.  This was
clarified in one of the ACPI 5.0A spec - the field has been renamed to
"displayed", so as not to confuse people.  Here's what the current ACPI
spec says:

Name    Size    Offset  Description
Status  1       38      1-byte status field indicating current status
                        about the table.
                        Bits [7:3] = Reserved (must be zero)
                        Bits [2:1] = Orientation Offset. These bits
                                     describe the clockwise degree
                                     offset from the image’s default
                                     orientation.
                                     [00] = 0, no offset
                                     [01] = 90
                                     [10] = 180
                                     [11] = 270

And I think 6.1 or 6.1A has accidentally dropped the text:

                        Bit [0] = Displayed. A one indicates the boot
                                  image graphic is displayed.

Further down it says:

        Bit 0 - Displayed
        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 while the image resource is displayed on the
        screen, and set to 0 while it is not displayed.

Clearly there has been some editing error going on here, and I'll send
in an ECR to fix that up.  It might be worth exposing "orientation" and
"displayed" directly instead of just exposing "status".

Aside from that an Lorenzo's other observations, the patch looks
reasonable to me.
Peter Jones - Jan. 29, 2019, 4:11 p.m.
On Wed, Jan 23, 2019 at 10:44:47AM +0100, Ard Biesheuvel wrote:
> On Wed, 23 Jan 2019 at 06:31, Dave Young <dyoung@redhat.com> wrote:
> > On 01/22/19 at 04:06pm, Ard Biesheuvel wrote:
> > > The bitmap left in the framebuffer by the firmware is described by an
> > > ACPI table called "BGRT", which describes the size, pixel format and
> > > the address of a BMP image in memory. While the BGRT ACPI table is
> > > guaranteed to reside in a "ACPI reclaim" memory region, which is
> > > never touched by Linux. The BMP image, however, typically resides
> > > in EFI Boot Services Memory, which may have been overwritten by the
> > > time the BGRT discovery routine runs.
> >
> > I vaguely remember boot service memory is reserved on X86,  so this
> > is an issue of arm only?
> 
> No both ARM and x86 are affected. Harry and/or Peter should be able to
> comment on the details.

I've definitely seen an x86 machine with this issue - Michael Kinney has
given me firmware for a MinnowBoard-MAX that results in having this
issue.  I'm don't recall if the image was in boot services memory or
not, but if we need I can test it tomorrow (I'm at my desk at work, it's
at my desk at home.)  In any case, whether or not it was clobbered
seemed to be mostly about the interaction between the memory layout the
firmware picked and luck on the order of what we allocate in kernel.
Ard Biesheuvel - Feb. 9, 2019, 1:41 p.m.
(+ Mike)

On Wed, 23 Jan 2019 at 10:44, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Wed, 23 Jan 2019 at 06:31, Dave Young <dyoung@redhat.com> wrote:
> >
> > Hi,
> >
> > On 01/22/19 at 04:06pm, Ard Biesheuvel wrote:
> > > The bitmap left in the framebuffer by the firmware is described by an
> > > ACPI table called "BGRT", which describes the size, pixel format and
> > > the address of a BMP image in memory. While the BGRT ACPI table is
> > > guaranteed to reside in a "ACPI reclaim" memory region, which is
> > > never touched by Linux. The BMP image, however, typically resides
> > > in EFI Boot Services Memory, which may have been overwritten by the
> > > time the BGRT discovery routine runs.
> >
> > I vaguely remember boot service memory is reserved on X86,  so this
> > is an issue of arm only?
> >
>
> No both ARM and x86 are affected. Harry and/or Peter should be able to
> comment on the details.
>

OK, so ARM is definitely affected, but after having had to revert this
patch since it breaks x86, I am looking into more detail and I have
trouble figuring *why* this would fail on x86 since, as Dave points
out, the x86 Linux kernel does not release boot services data regions
for general allocation until much later.

Mike (or Peter), do you happen to have a Linux boot log of the case
where the BGRT is being corrupted?

Patch

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 44e3c351e1ea..7429a811f76d 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -230,8 +230,6 @@  void __init acpi_boot_table_init(void)
 			early_init_dt_scan_chosen_stdout();
 	} else {
 		acpi_parse_spcr(earlycon_acpi_spcr_enable, true);
-		if (IS_ENABLED(CONFIG_ACPI_BGRT))
-			acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
 	}
 }
 
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 2624de16cd7a..2d3535b62752 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -1633,8 +1633,6 @@  int __init acpi_boot_init(void)
 	acpi_process_madt();
 
 	acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet);
-	if (IS_ENABLED(CONFIG_ACPI_BGRT))
-		acpi_table_parse(ACPI_SIG_BGRT, acpi_parse_bgrt);
 
 	if (!acpi_noirq)
 		x86_init.pci.init = pci_acpi_init;
diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c
index 75af78361ce5..048413e06898 100644
--- a/drivers/acpi/bgrt.c
+++ b/drivers/acpi/bgrt.c
@@ -81,12 +81,6 @@  static const struct attribute_group bgrt_attribute_group = {
 	.bin_attrs = bgrt_bin_attributes,
 };
 
-int __init acpi_parse_bgrt(struct acpi_table_header *table)
-{
-	efi_bgrt_init(table);
-	return 0;
-}
-
 static int __init bgrt_init(void)
 {
 	int ret;
diff --git a/drivers/firmware/efi/efi-bgrt.c b/drivers/firmware/efi/efi-bgrt.c
index b22ccfb0c991..8bd9b96942d2 100644
--- a/drivers/firmware/efi/efi-bgrt.c
+++ b/drivers/firmware/efi/efi-bgrt.c
@@ -27,24 +27,92 @@  struct bmp_header {
 	u32 size;
 } __packed;
 
-void __init efi_bgrt_init(struct acpi_table_header *table)
+void __init efi_bgrt_init(unsigned long rsdp_phys)
 {
 	void *image;
 	struct bmp_header bmp_header;
 	struct acpi_table_bgrt *bgrt = &bgrt_tab;
+	struct acpi_table_bgrt *table = NULL;
+	struct acpi_table_rsdp *rsdp;
+	struct acpi_table_header *hdr;
+	u64 xsdt_phys;
+	u32 rsdt_phys;
+	unsigned int len;
 
-	if (acpi_disabled)
+	if (!efi_enabled(EFI_MEMMAP))
 		return;
 
-	if (!efi_enabled(EFI_MEMMAP))
+	/* map the root pointer table to find the xsdt/rsdt values */
+	rsdp = early_memremap(rsdp_phys, sizeof(*rsdp));
+	if (rsdp && ACPI_VALIDATE_RSDP_SIG(rsdp->signature)) {
+		xsdt_phys = rsdp->xsdt_physical_address;
+		rsdt_phys = rsdp->rsdt_physical_address;
+	} else {
+		WARN_ON(1);
+		return;
+	}
+	early_memunmap(rsdp, sizeof(*rsdp));
+
+	/* obtain the length of whichever table we will be using */
+	hdr = early_memremap(xsdt_phys ?: rsdt_phys, sizeof(*hdr));
+	if (WARN_ON(!hdr))
+		return;
+	len = hdr->length;
+	early_memunmap(hdr, sizeof(*hdr));
+
+	if (xsdt_phys) {
+		struct acpi_table_xsdt *xsdt = early_memremap(xsdt_phys, len);
+		int i;
+
+		if (WARN_ON(!xsdt))
+			return;
+
+		for (i = 0; i < (len - sizeof(*hdr)) / sizeof(u64); i++) {
+			table = early_memremap(xsdt->table_offset_entry[i],
+					       sizeof(*table));
+			if (WARN_ON(!table))
+				break;
+
+			if (ACPI_COMPARE_NAME(table->header.signature,
+					      ACPI_SIG_BGRT))
+				break;
+			early_memunmap(table, sizeof(*table));
+			table = NULL;
+		}
+		early_memunmap(xsdt, len);
+	} else if (rsdt_phys) {
+		struct acpi_table_rsdt *rsdt = early_memremap(rsdt_phys, len);
+		int i;
+
+		if (WARN_ON(!rsdt))
+			return;
+
+		for (i = 0; i < (len - sizeof(*hdr)) / sizeof(u32); i++) {
+			table = early_memremap(rsdt->table_offset_entry[i],
+					       sizeof(*table));
+			if (WARN_ON(!table))
+				break;
+
+			if (ACPI_COMPARE_NAME(table->header.signature,
+					      ACPI_SIG_BGRT))
+				break;
+			early_memunmap(table, sizeof(*table));
+			table = NULL;
+		}
+		early_memunmap(rsdt, len);
+	}
+
+	if (!table)
 		return;
 
-	if (table->length < sizeof(bgrt_tab)) {
+	if (table->header.length < sizeof(bgrt_tab)) {
 		pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n",
-		       table->length, sizeof(bgrt_tab));
+		       table->header.length, sizeof(bgrt_tab));
 		return;
 	}
-	*bgrt = *(struct acpi_table_bgrt *)table;
+	*bgrt = *table;
+	early_memunmap(table, sizeof(*table));
+
 	if (bgrt->version != 1) {
 		pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n",
 		       bgrt->version);
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 4c46ff6f2242..e5ef5c0eacc1 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -20,6 +20,7 @@ 
 #include <linux/init.h>
 #include <linux/device.h>
 #include <linux/efi.h>
+#include <linux/efi-bgrt.h>
 #include <linux/of.h>
 #include <linux/of_fdt.h>
 #include <linux/io.h>
@@ -592,6 +593,18 @@  int __init efi_config_parse_tables(void *config_tables, int count, int sz,
 
 		early_memunmap(tbl, sizeof(*tbl));
 	}
+
+	/*
+	 * We need to parse the BGRT table (which is an ACPI table not a UEFI
+	 * configuration table) by hand and figure out where the bitmap it
+	 * describes lives in memory so we can reserve it early on. Otherwise,
+	 * it may be clobbered by the time we get to it during the ordinary ACPI
+	 * table init sequence.
+	 */
+	if (IS_ENABLED(CONFIG_ACPI_BGRT) &&
+	    efi.acpi20 != EFI_INVALID_TABLE_ADDR)
+		efi_bgrt_init(efi.acpi20);
+
 	return 0;
 }
 
diff --git a/include/linux/efi-bgrt.h b/include/linux/efi-bgrt.h
index e6cd51005633..528ea62d99ec 100644
--- a/include/linux/efi-bgrt.h
+++ b/include/linux/efi-bgrt.h
@@ -6,7 +6,7 @@ 
 
 #ifdef CONFIG_ACPI_BGRT
 
-void efi_bgrt_init(struct acpi_table_header *table);
+void efi_bgrt_init(unsigned long rsdp_phys);
 int __init acpi_parse_bgrt(struct acpi_table_header *table);
 
 /* The BGRT data itself; only valid if bgrt_image != NULL. */
@@ -15,7 +15,7 @@  extern struct acpi_table_bgrt bgrt_tab;
 
 #else /* !CONFIG_ACPI_BGRT */
 
-static inline void efi_bgrt_init(struct acpi_table_header *table) {}
+static inline void efi_bgrt_init(unsigned long rsdp_phys) {}
 static inline int __init acpi_parse_bgrt(struct acpi_table_header *table)
 {
 	return 0;