[PATCH V4 0/4] map GHES memory region according to EFI memory map

[PATCH V4 0/4] map GHES memory region according to EFI memory map

[Jonathan (Zhixiong) Zhang]

From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>

On a platform with APEI (ACPI Platform Error Interface) enabled, firmware
updates a memory region with hardware error record using nocache
attribute. When OS reads the region, since it maps the region with
cacahed attribute even though EFI memory map defines this region as
uncached, OS gets stale data and errorneously reports there is no new
HW error.

When ghes driver maps the memory region, it uses the cache attribute
according to EFI memory map, if EFI memory map feature is enabled
at runtime.

Since both arch/x86 and arch/ia64 implemented architecture agnostic EFI
memory map attribue lookup function efi_memattributes(), the code is
moved from arch/x86 into EFI subsystem and is declared as __weak; archs
other than ia64 should not override the default implementation.

V4:
1. Introduced arch_apei_get_mem_attributes() to allow arch specific
   implementation of getting pgprot_t appropriate for a physical
   address.
2. Implemented arch_apei_get_mem_attributes() for x86 and for arm64.

V3:
1. Rebased to v4.1-rc7.
2. Moved efi_mem_attributes() from arch/x86 to drivers/firmware/efi
   and declared it as __weak.
3. Introduced ARCH_APEI_PAGE_KERNEL_UC to allow arch specific page
   protection type for UC.
4. Removed efi_ioremap(). It can not be used for GHES memory region
   mapping purpose since ioremap can not be used in atomic context.

V2:
1. Rebased to v4.1-rc5.
2. Split removal of efi_mem_attributes() and creation of efi_ioremap()
into two patches.

Jonathan (Zhixiong) Zhang (4):
  efi: x86: rearrange efi_mem_attributes()
  x86: acpi: add arch_apei_get_mem_attributes()
  arm64: apei: implement arch_apei_get_mem_attributes()
  acpi, apei: use appropriate pgprot_t to map GHES memory

 arch/arm64/kernel/Makefile  |  1 +
 arch/arm64/kernel/apei.c    | 27 +++++++++++++++++++++++++++
 arch/x86/kernel/acpi/apei.c | 10 ++++++++++
 arch/x86/platform/efi/efi.c | 18 ------------------
 drivers/acpi/apei/ghes.c    |  6 ++++--
 drivers/firmware/efi/efi.c  | 18 ++++++++++++++++++
 include/acpi/apei.h         |  1 +
 7 files changed, 61 insertions(+), 20 deletions(-)
 create mode 100644 arch/arm64/kernel/apei.c

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH V4 1/4] efi: x86: rearrange efi_mem_attributes()

[Jonathan (Zhixiong) Zhang]

From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>

x86 and ia64 implement efi_mem_attributes() differently. This
function needs to be available for other arch (such as arm64)
as well, such as for the purpose of ACPI/APEI.

ia64 efi does not setup memmap variable and does not set
EFI_MEMMAP flag, so it needs to have its unique implementation
of efi_mem_attributes().

Move efi_mem_attributes() implementation from x86 to efi, and
declare it with __weak. It is recommended that other archs
should not override the default implementation.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
---
 arch/x86/platform/efi/efi.c | 18 ------------------
 drivers/firmware/efi/efi.c  | 18 ++++++++++++++++++
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 02744df576d5..10bd5289c593 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -926,24 +926,6 @@ u32 efi_mem_type(unsigned long phys_addr)
 	return 0;
 }
 
-u64 efi_mem_attributes(unsigned long phys_addr)
-{
-	efi_memory_desc_t *md;
-	void *p;
-
-	if (!efi_enabled(EFI_MEMMAP))
-		return 0;
-
-	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
-		md = p;
-		if ((md->phys_addr <= phys_addr) &&
-		    (phys_addr < (md->phys_addr +
-				  (md->num_pages << EFI_PAGE_SHIFT))))
-			return md->attribute;
-	}
-	return 0;
-}
-
 static int __init arch_parse_efi_cmdline(char *str)
 {
 	if (parse_option_str(str, "old_map"))
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3061bb8629dc..88531b0dee80 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -517,3 +517,21 @@ char * __init efi_md_typeattr_format(char *buf, size_t size,
 			 attr & EFI_MEMORY_UC      ? "UC"  : "");
 	return buf;
 }
+
+u64 __weak efi_mem_attributes(unsigned long phys_addr)
+{
+	efi_memory_desc_t *md;
+	void *p;
+
+	if (!efi_enabled(EFI_MEMMAP))
+		return 0;
+
+	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
+		md = p;
+		if ((md->phys_addr <= phys_addr) &&
+		    (phys_addr < (md->phys_addr +
+		    (md->num_pages << EFI_PAGE_SHIFT))))
+			return md->attribute;
+	}
+	return 0;
+}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH V4 2/4] x86: acpi: add arch_apei_get_mem_attributes()

[Jonathan (Zhixiong) Zhang]

From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>

... to allow arch specific implementation of getting page
protection type associated with a physical address.

If the physical address has memory attributes defined by EFI
memmap as EFI_MEMORY_UC, the page protection type is
PAGE_KENERL_NOCACHE. Otherwise, the page protection type is
PAGE_KERNEL.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
---
 arch/x86/kernel/acpi/apei.c | 10 ++++++++++
 include/acpi/apei.h         |  1 +
 2 files changed, 11 insertions(+)

diff --git a/arch/x86/kernel/acpi/apei.c b/arch/x86/kernel/acpi/apei.c
index c280df6b2aa2..cfcde9142b94 100644
--- a/arch/x86/kernel/acpi/apei.c
+++ b/arch/x86/kernel/acpi/apei.c
@@ -14,6 +14,8 @@
 
 #include <acpi/apei.h>
 
+#include <linux/efi.h>
+
 #include <asm/mce.h>
 #include <asm/tlbflush.h>
 
@@ -60,3 +62,11 @@ void arch_apei_flush_tlb_one(unsigned long addr)
 {
 	__flush_tlb_one(addr);
 }
+
+pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
+{
+	if (efi_mem_attributes(addr) & EFI_MEMORY_UC)
+		return PAGE_KERNEL_NOCACHE;
+	else
+		return PAGE_KERNEL;
+}
diff --git a/include/acpi/apei.h b/include/acpi/apei.h
index 76284bb560a6..4dd8c490e24f 100644
--- a/include/acpi/apei.h
+++ b/include/acpi/apei.h
@@ -45,6 +45,7 @@ int erst_clear(u64 record_id);
 int arch_apei_enable_cmcff(struct acpi_hest_header *hest_hdr, void *data);
 void arch_apei_report_mem_error(int sev, struct cper_sec_mem_err *mem_err);
 void arch_apei_flush_tlb_one(unsigned long addr);
+pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr);
 
 #endif
 #endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V4 1/4] efi: x86: rearrange efi_mem_attributes()

[Matt Fleming]

On Fri, 12 Jun, at 06:01:44PM, Jonathan (Zhixiong) Zhang wrote:
> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
> 
> x86 and ia64 implement efi_mem_attributes() differently. This
> function needs to be available for other arch (such as arm64)
> as well, such as for the purpose of ACPI/APEI.
> 
> ia64 efi does not setup memmap variable and does not set
> EFI_MEMMAP flag, so it needs to have its unique implementation
> of efi_mem_attributes().

Something like the above paragraph...
 
> @@ -517,3 +517,21 @@ char * __init efi_md_typeattr_format(char *buf, size_t size,
>  			 attr & EFI_MEMORY_UC      ? "UC"  : "");
>  	return buf;
>  }

needs to go here, i.e. as documentation for efi_mem_atributes(). You
need to explain why this function is marked as __weak at the function
definition site.

> +u64 __weak efi_mem_attributes(unsigned long phys_addr)
> +{
> +	efi_memory_desc_t *md;
> +	void *p;
> +
> +	if (!efi_enabled(EFI_MEMMAP))
> +		return 0;
> +
> +	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> +		md = p;
> +		if ((md->phys_addr <= phys_addr) &&
> +		    (phys_addr < (md->phys_addr +
> +		    (md->num_pages << EFI_PAGE_SHIFT))))
> +			return md->attribute;
> +	}
> +	return 0;
> +}

because otherwise people are going to read this in the future and think,

 "efi_mem_attribute() doesn't quite work how I want it to for my arch,
 but it's __weak, so I'll override it"

which is absolutely not the practice we should be promoting.

How about something like this,

	/*
	 * efi_mem_attributes - lookup memmap attributes for physical address
	 * @phys_addr: the physical address to lookup
	 *
	 * Search in the EFI memory map for the region covering
	 * @phys_addr. Returns the EFI memory attributes if the region
	 * was found in the memory map, 0 otherwise.
	 *
	 * Despite being marked __weak most architectures should *not*
	 * override this function. It is __weak solely for the benefit
	 * of ia64 which has a funky EFI memory map that doesn't work
	 * the same way as other architectures.
	 */

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V4 1/4] efi: x86: rearrange efi_mem_attributes()

[Zhang, Jonathan Zhixiong]

On 6/15/2015 10:09 AM, Matt Fleming wrote:
> On Fri, 12 Jun, at 06:01:44PM, Jonathan (Zhixiong) Zhang wrote:
>> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>
>> x86 and ia64 implement efi_mem_attributes() differently. This
>> function needs to be available for other arch (such as arm64)
>> as well, such as for the purpose of ACPI/APEI.
>>
>> ia64 efi does not setup memmap variable and does not set
>> EFI_MEMMAP flag, so it needs to have its unique implementation
>> of efi_mem_attributes().
>
> Something like the above paragraph...
>
>> @@ -517,3 +517,21 @@ char * __init efi_md_typeattr_format(char *buf, size_t size,
>>   			 attr & EFI_MEMORY_UC      ? "UC"  : "");
>>   	return buf;
>>   }
>
> needs to go here, i.e. as documentation for efi_mem_atributes(). You
> need to explain why this function is marked as __weak at the function
> definition site.
>
>> +u64 __weak efi_mem_attributes(unsigned long phys_addr)
>> +{
>> +	efi_memory_desc_t *md;
>> +	void *p;
>> +
>> +	if (!efi_enabled(EFI_MEMMAP))
>> +		return 0;
>> +
>> +	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
>> +		md = p;
>> +		if ((md->phys_addr <= phys_addr) &&
>> +		    (phys_addr < (md->phys_addr +
>> +		    (md->num_pages << EFI_PAGE_SHIFT))))
>> +			return md->attribute;
>> +	}
>> +	return 0;
>> +}
>
> because otherwise people are going to read this in the future and think,
>
>   "efi_mem_attribute() doesn't quite work how I want it to for my arch,
>   but it's __weak, so I'll override it"
>
> which is absolutely not the practice we should be promoting.
>
> How about something like this,
>
> 	/*
> 	 * efi_mem_attributes - lookup memmap attributes for physical address
> 	 * @phys_addr: the physical address to lookup
> 	 *
> 	 * Search in the EFI memory map for the region covering
> 	 * @phys_addr. Returns the EFI memory attributes if the region
> 	 * was found in the memory map, 0 otherwise.
> 	 *
> 	 * Despite being marked __weak most architectures should *not*
> 	 * override this function. It is __weak solely for the benefit
> 	 * of ia64 which has a funky EFI memory map that doesn't work
> 	 * the same way as other architectures.
> 	 */
>
Thank you Matt for the suggestion. Will do so accordingly.

-- 
Jonathan (Zhixiong) Zhang
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project