[PATCH 0/2] ACPI, x86/efi: Remove ACPI BGRT tables for kexec

[PATCH 0/2] ACPI, x86/efi: Remove ACPI BGRT tables for kexec

[Matt Fleming]

Based on Dave's report that the BGRT image regions are being accessed
on kexec reboot (which by that time contain garbage),

  https://lkml.kernel.org/r/20160127112044.GA2961@dhcp-128-65.nay.redhat.com

the following patches simply delete the table when doing a kexec boot.

This is part of a wider compaign to stop kexec-specific code from
leaking into all parts of arch/x86. Instead of sprinkling "if
(efi_setup)", kexec should be shaping the platform information to more
accurately describe which features are still available, so that
existing drivers work transparently without kexec quirks.

Matt Fleming (2):
  ACPICA: Tables: Add function to remove ACPI tables
  x86/efi: Delete ACPI BGRT when booting via kexec

 arch/x86/include/asm/efi.h    |  1 +
 arch/x86/kernel/setup.c       |  4 +++-
 arch/x86/platform/efi/efi.c   | 38 ++++++++++++++++++++++++++++++
 drivers/acpi/acpica/tbxface.c | 54 +++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpixf.h         |  3 +++
 5 files changed, 99 insertions(+), 1 deletion(-)

-- 
2.6.2

[PATCH 1/2] ACPICA: Tables: Add function to remove ACPI tables

[Matt Fleming]

There are existing internal functions that allow the removal of ACPI
tables, but they're not exposed to the OS in any useful way.

Introduce acpi_remove_table() which allows tables to be invalidated in
the global table list, resulting in failure of subsequent calls to
acpi_get_table() for those tables.

The rationale for this change is the ability to remove the BGRT table
during kexec boot. The BGRT table refers to memory regions that are no
longer reserved by the firmware once the kexec kernel boots, having
been released for general allocation by the previous kernel.

Cc: Dave Young <dyoung@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: <kexec@lists.infradead.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/acpi/acpica/tbxface.c | 54 +++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpixf.h         |  3 +++
 2 files changed, 57 insertions(+)

diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
index 326df65decef..999eecd89601 100644
--- a/drivers/acpi/acpica/tbxface.c
+++ b/drivers/acpi/acpica/tbxface.c
@@ -480,3 +480,57 @@ cleanup:
 }
 
 ACPI_EXPORT_SYMBOL(acpi_remove_table_handler)
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_remove_table
+ *
+ * PARAMETERS:  signature           - ACPI signature of needed table
+ *              instance            - Which instance (for SSDTs)
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Finds and removes an ACPI table.
+ *
+ ******************************************************************************/
+acpi_status acpi_remove_table(char *signature, u32 instance)
+{
+	struct acpi_table_desc *table_desc;
+	acpi_status status;
+	u32 i;
+	u32 j;
+
+	/* Parameter validation */
+	if (!signature) {
+		return (AE_BAD_PARAMETER);
+	}
+
+	/* Walk the root table list */
+
+	for (i = 0, j = 0; i < acpi_gbl_root_table_list.current_table_count;
+	     i++) {
+		if (!ACPI_COMPARE_NAME
+		    (&(acpi_gbl_root_table_list.tables[i].signature),
+		     signature)) {
+			continue;
+		}
+
+		if (++j < instance) {
+			continue;
+		}
+
+		table_desc = &acpi_gbl_root_table_list.tables[i];
+
+		status = acpi_tb_validate_table(table_desc);
+		if (ACPI_FAILURE(status)) {
+			return (status);
+		}
+
+		acpi_tb_uninstall_table(table_desc);
+		return (AE_OK);
+	}
+
+	return (AE_NOT_FOUND);
+}
+
+ACPI_EXPORT_SYMBOL(acpi_remove_table);
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index c96621e87c19..47e51612293e 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -505,6 +505,9 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status
 ACPI_EXTERNAL_RETURN_STATUS(acpi_status
 			     acpi_remove_table_handler(acpi_table_handler
 						       handler))
+ACPI_EXTERNAL_RETURN_STATUS(acpi_status
+			     acpi_remove_table(acpi_string signature,
+					       u32 instance))
 
 /*
  * Namespace and name interfaces
-- 
2.6.2

RE: [PATCH 1/2] ACPICA: Tables: Add function to remove ACPI tables

[Zheng, Lv]

Hi, 

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Matt Fleming
[Lv Zheng] 
First, is it possible for you to submit an ACPICA patch instead of a Linux patch.
The reasoning for doing this can be found at:
https://acpica.org/Licensing

> Subject: [PATCH 1/2] ACPICA: Tables: Add function to remove ACPI tables
> 
> There are existing internal functions that allow the removal of ACPI
> tables, but they're not exposed to the OS in any useful way.
> 
> Introduce acpi_remove_table() which allows tables to be invalidated in
> the global table list, resulting in failure of subsequent calls to
> acpi_get_table() for those tables.
> 
> The rationale for this change is the ability to remove the BGRT table
> during kexec boot. The BGRT table refers to memory regions that are no
> longer reserved by the firmware once the kexec kernel boots, having
> been released for general allocation by the previous kernel.
> 
> Cc: Dave Young <dyoung@redhat.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Josh Triplett <josh@joshtriplett.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: <kexec@lists.infradead.org>
> Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> ---
>  drivers/acpi/acpica/tbxface.c | 54
> +++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/acpixf.h         |  3 +++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c
> index 326df65decef..999eecd89601 100644
> --- a/drivers/acpi/acpica/tbxface.c
> +++ b/drivers/acpi/acpica/tbxface.c
> @@ -480,3 +480,57 @@ cleanup:
>  }
> 
>  ACPI_EXPORT_SYMBOL(acpi_remove_table_handler)
> +
> +/**************************************************************
> *****************
> + *
> + * FUNCTION:    acpi_remove_table
[Lv Zheng] 
I'd prefer acpi_uninstall_table() in order to keep the consistencies of the function naming.
There is in fact a state machine defined by the table manager design:
UNINSTALLED - install -> INSTALLED/INVALIDATED - validate -> VALIDATED/UNLOADED - load -> LOADED
LOADED -> unload -> UNLOADED/VALIDATED -> invalidate - INVALIDATED/INSTALLED -> uninstall -> UNINSTALLED

> + *
> + * PARAMETERS:  signature           - ACPI signature of needed table
> + *              instance            - Which instance (for SSDTs)
> + *
> + * RETURN:      Status
> + *
> + * DESCRIPTION: Finds and removes an ACPI table.
> + *
> +
> ****************************************************************
> **************/
> +acpi_status
[Lv Zheng] 
You need to put __init here.
Otherwise another API (for example, acpi_unload_table()) should be provided instead of this.

> acpi_remove_table(char *signature, u32 instance)
[Lv Zheng] 
I'm not sure if using instance is a good idea here.
We have extensions not upstreamed that have something to do with the table indexing.
But well, it doesn't look bad for now.

> +{
> +	struct acpi_table_desc *table_desc;
> +	acpi_status status;
> +	u32 i;
> +	u32 j;
> +
> +	/* Parameter validation */
> +	if (!signature) {
> +		return (AE_BAD_PARAMETER);
> +	}
> +
> +	/* Walk the root table list */
> +
> +	for (i = 0, j = 0; i < acpi_gbl_root_table_list.current_table_count;
> +	     i++) {
> +		if (!ACPI_COMPARE_NAME
> +		    (&(acpi_gbl_root_table_list.tables[i].signature),
> +		     signature)) {
> +			continue;
> +		}
[Lv Zheng] 
After removing a table, the instance no remains 2 for the next table of the same signature.
Is it intentional?

> +
> +		if (++j < instance) {
> +			continue;
> +		}
> +
> +		table_desc = &acpi_gbl_root_table_list.tables[i];
> +
> +		status = acpi_tb_validate_table(table_desc);
> +		if (ACPI_FAILURE(status)) {
> +			return (status);
> +		}
[Lv Zheng] 
You needn't invoke acpi_tb_validate_table() here.

> +
> +		acpi_tb_uninstall_table(table_desc);
> +		return (AE_OK);
> +	}
> +
> +	return (AE_NOT_FOUND);
> +}
> +
> +ACPI_EXPORT_SYMBOL(acpi_remove_table);
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index c96621e87c19..47e51612293e 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -505,6 +505,9 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status
>  ACPI_EXTERNAL_RETURN_STATUS(acpi_status
>  			     acpi_remove_table_handler(acpi_table_handler
>  						       handler))
> +ACPI_EXTERNAL_RETURN_STATUS(acpi_status
> +			     acpi_remove_table(acpi_string signature,
> +					       u32 instance))
> 
[Lv Zheng] 
It is better to move this declaration close to acpi_install_table().

Thanks and best regards
-Lv

>  /*
>   * Namespace and name interfaces
> --
> 2.6.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ACPICA: Tables: Add function to remove ACPI tables

[Rafael J. Wysocki]

On Thu, Feb 18, 2016 at 3:34 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi,
>
>> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>> owner@vger.kernel.org] On Behalf Of Matt Fleming
> [Lv Zheng]
> First, is it possible for you to submit an ACPICA patch instead of a Linux patch.
> The reasoning for doing this can be found at:
> https://acpica.org/Licensing

Actually, the reason is that, as a rule, the process for ACPICA
patches is that they first go to upstream ACPICA and they are acquired
by Linux from there.

While there are some exceptions from that process, there also are good
reasons for that process to be followed, including the licensing one
mentioned by Lv.

All that said, Matt, if you agree that the patch can be applied under
the BSD license, I think we can offer help with converting it to the
upstream ACPICA coding conventions and applying it there.  Lv, would
you be able to take care of that?

Thanks,
Rafael

Re: [PATCH 1/2] ACPICA: Tables: Add function to remove ACPI tables

[Matt Fleming]

On Thu, 18 Feb, at 09:15:28PM, Rafael J. Wysocki wrote:
> 
> Actually, the reason is that, as a rule, the process for ACPICA
> patches is that they first go to upstream ACPICA and they are acquired
> by Linux from there.
> 
> While there are some exceptions from that process, there also are good
> reasons for that process to be followed, including the licensing one
> mentioned by Lv.
> 
> All that said, Matt, if you agree that the patch can be applied under
> the BSD license, I think we can offer help with converting it to the
> upstream ACPICA coding conventions and applying it there.  Lv, would
> you be able to take care of that?

I don't have any problem with that, but can we hold off on this patch
for now? There's another approach to fixing the BGRT issue with kexec
that's being discussed which would supersede this,

  https://lkml.kernel.org/r/20160218141544.GH2651@codeblueprint.co.uk

Assuming this patch does get picked up again, I'm happy to respin it
against upstream ACPICA, but how do I go about getting dependent
patches merged, PATCH 2/2 in this case?

Re: [PATCH 1/2] ACPICA: Tables: Add function to remove ACPI tables

[Rafael J. Wysocki]

On Thu, Feb 18, 2016 at 9:41 PM, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Thu, 18 Feb, at 09:15:28PM, Rafael J. Wysocki wrote:
>>
>> Actually, the reason is that, as a rule, the process for ACPICA
>> patches is that they first go to upstream ACPICA and they are acquired
>> by Linux from there.
>>
>> While there are some exceptions from that process, there also are good
>> reasons for that process to be followed, including the licensing one
>> mentioned by Lv.
>>
>> All that said, Matt, if you agree that the patch can be applied under
>> the BSD license, I think we can offer help with converting it to the
>> upstream ACPICA coding conventions and applying it there.  Lv, would
>> you be able to take care of that?
>
> I don't have any problem with that, but can we hold off on this patch
> for now? There's another approach to fixing the BGRT issue with kexec
> that's being discussed which would supersede this,
>
>   https://lkml.kernel.org/r/20160218141544.GH2651@codeblueprint.co.uk
>
> Assuming this patch does get picked up again, I'm happy to respin it
> against upstream ACPICA, but how do I go about getting dependent
> patches merged, PATCH 2/2 in this case?

We generate a Linux version of the patch out of the upstream ACPICA
sources (semi-automatically) and that can be merged into Linux in
advance.  We don't do that as a rule, but it can be done.  That at
least ensures that we'll be consistent with future ACPICA updates from
the upstream.

Thanks,
Rafael

RE: [PATCH 1/2] ACPICA: Tables: Add function to remove ACPI tables

[Zheng, Lv]

Hi,

> From: Matt Fleming [mailto:matt@codeblueprint.co.uk]
> Subject: Re: [PATCH 1/2] ACPICA: Tables: Add function to remove ACPI tables
> 
> On Thu, 18 Feb, at 09:15:28PM, Rafael J. Wysocki wrote:
> >
> > Actually, the reason is that, as a rule, the process for ACPICA
> > patches is that they first go to upstream ACPICA and they are acquired
> > by Linux from there.
> >
> > While there are some exceptions from that process, there also are good
> > reasons for that process to be followed, including the licensing one
> > mentioned by Lv.
> >
> > All that said, Matt, if you agree that the patch can be applied under
> > the BSD license, I think we can offer help with converting it to the
> > upstream ACPICA coding conventions and applying it there.  Lv, would
> > you be able to take care of that?
> 
> I don't have any problem with that, but can we hold off on this patch
> for now? There's another approach to fixing the BGRT issue with kexec
> that's being discussed which would supersede this,
> 
>   https://lkml.kernel.org/r/20160218141544.GH2651@codeblueprint.co.uk
> 
> Assuming this patch does get picked up again, I'm happy to respin it
> against upstream ACPICA, but how do I go about getting dependent
> patches merged, PATCH 2/2 in this case?
[Lv Zheng] 
I can help to port your code to the ACPICA upstream.
Rafael can help to merge the linuxized ported patch and the PATCH 2/2 if you want this solution to be upstreamed now.
Or you can wait and submit PATCH 2/2 again.
PATCH 1/2 should be a part of ACPICA 201603xx release I think.

Thanks and best regards
-Lv

[PATCH 2/2] x86/efi: Delete ACPI BGRT when booting via kexec

[Matt Fleming]

Dave reports that for kexec reboot the ACPI BGRT image region may
contain garbage data, because the image lives in EFI Boot Services
regions that were released and freed when the first kernel called
efi_free_boot_services().

Since the EFI Boot Services regions can be large (multiple gigabytes)
preserving them throughout the kernel's lifetime and across kexec
reboot is not a viable solution. Instead we need to avoid accessing
the BGRT image regions under kexec.

Rather than dirtying the ACPI BGRT driver with conditionals that check
whether we're booting via kexec, we can execute the existing code path
that exits if the table cannot be found - by removing the BGRT table.

It is unfortunate that this logic cannot be folded into the existing
kexec-specific EFI code, but there are dependencies on having loaded
the ACPI tables, which happens much later.

Reported-by: Dave Young <dyoung@redhat.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Josh Triplett <josh@joshtriplett.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: <kexec@lists.infradead.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 arch/x86/include/asm/efi.h  |  1 +
 arch/x86/kernel/setup.c     |  4 +++-
 arch/x86/platform/efi/efi.c | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 7bb206f73915..0e813da58be6 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -146,6 +146,7 @@ extern void __init efi_dump_pagetable(void);
 extern void __init efi_apply_memmap_quirks(void);
 extern int __init efi_reuse_config(u64 tables, int nr_tables);
 extern void efi_delete_dummy_variable(void);
+extern void __init efi_kexec_remove_acpi_tables(void);
 
 struct efi_setup_data {
 	u64 fw_vendor;
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index d3d80e6d42a2..8131962ce8c2 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1249,8 +1249,10 @@ void __init setup_arch(char **cmdline_p)
 	register_refined_jiffies(CLOCK_TICK_RATE);
 
 #ifdef CONFIG_EFI
-	if (efi_enabled(EFI_BOOT))
+	if (efi_enabled(EFI_BOOT)) {
 		efi_apply_memmap_quirks();
+		efi_kexec_remove_acpi_tables();
+	}
 #endif
 }
 
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 39748d228e34..d87f42697ad0 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -485,6 +485,44 @@ static int __init efi_kexec_override(void)
 	return 0;
 }
 
+/*
+ * The BGRT table, if present, refers to memory regions that are no
+ * longer reserved during kexec boot, having been released by the
+ * previous kernel. The BGRT image likely contains garbage.
+ *
+ * Delete the ACPI table, since it is useless for kexec, and so that
+ * it is impossible for the ACPI BGRT driver to distinguish between
+ * "Platform has no BGRT" and "We were booted via kexec".
+ *
+ * Note that this function must only be called after the ACPI tables
+ * have been initialised.
+ */
+void __init efi_kexec_remove_acpi_tables(void)
+{
+	struct acpi_table_header *header;
+	acpi_status status;
+
+	if (!efi_setup)
+		return;
+
+	status = acpi_get_table(ACPI_SIG_BGRT, 0, &header);
+	if (ACPI_FAILURE(status))
+		return;
+
+	status = acpi_remove_table(ACPI_SIG_BGRT, 0);
+	if (ACPI_FAILURE(status)) {
+		pr_err("Failed to remove ACPI BGRT table\n");
+		return;
+	}
+
+	/*
+	 * Since we've probably already told the user that we have a
+	 * BGRT when parsing the ACPI tables, inform them that we have
+	 * intentionally removed it now.
+	 */
+	pr_info("Removed ACPI BGRT table for kexec\n");
+}
+
 void __init efi_init(void)
 {
 	efi_char16_t *c16;
-- 
2.6.2