[PATCH 0/2] map GHES memory region with EFI memory map

[PATCH 0/2] map GHES memory region with 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 cache 
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.

Since both arch/x86 and arc/ia64 implemented architecture agnostic EFI 
memory map attribue lookup function, the code is moved from arch/x86
and arch/ia64 into EFI subsystem.

Jonathan (Zhixiong) Zhang (2):
  efi: arch, x86: arch, ia64: rearrange EFI memmap related functions
  acpi, apei: use EFI memmap to map GHES memory

 arch/ia64/kernel/efi.c      | 11 -----------
 arch/x86/platform/efi/efi.c | 18 ------------------
 drivers/acpi/apei/ghes.c    | 13 +++++++++++++
 drivers/firmware/efi/efi.c  | 27 +++++++++++++++++++++++++++
 include/linux/efi.h         |  1 +
 5 files changed, 41 insertions(+), 29 deletions(-)

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

[PATCH 1/2] efi: arch, x86: arch, ia64: rearrange EFI memmap related functions

[Jonathan (Zhixiong) Zhang]

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

Both x86 and ia64 implemented efi_mem_attributs function, which is architecture
agnositc. This function is moved to efi subsystem.

efi_remap() function is added. If EFI memmap feature is enabled, and if a
memory region has attribute of EFI_MEMORY_UC, map it as uncached.

---
This patch was tested on an arm64 platform. It was built on x86 platform.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
---
 arch/ia64/kernel/efi.c      | 11 -----------
 arch/x86/platform/efi/efi.c | 18 ------------------
 drivers/firmware/efi/efi.c  | 27 +++++++++++++++++++++++++++
 include/linux/efi.h         |  1 +
 4 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
index c52d7540dc05..ef20ec784b04 100644
--- a/arch/ia64/kernel/efi.c
+++ b/arch/ia64/kernel/efi.c
@@ -771,17 +771,6 @@ efi_mem_type (unsigned long phys_addr)
 }
 
 u64
-efi_mem_attributes (unsigned long phys_addr)
-{
-	efi_memory_desc_t *md = efi_memory_descriptor(phys_addr);
-
-	if (md)
-		return md->attribute;
-	return 0;
-}
-EXPORT_SYMBOL(efi_mem_attributes);
-
-u64
 efi_mem_attribute (unsigned long phys_addr, unsigned long size)
 {
 	unsigned long end = phys_addr + size;
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index dbc8627a5cdf..88b3ebaeb72f 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -917,24 +917,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..5b42bb6d1fde 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -517,3 +517,30 @@ char * __init efi_md_typeattr_format(char *buf, size_t size,
 			 attr & EFI_MEMORY_UC      ? "UC"  : "");
 	return buf;
 }
+
+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;
+}
+
+void __iomem *efi_remap(phys_addr_t phys_addr, size_t size)
+{
+	if (efi_enabled(EFI_MEMMAP) &&
+	    (efi_mem_attributes(phys_addr) & EFI_MEMORY_UC))
+		return ioremap(phys_addr, size);
+	else
+		return ioremap_cache(phys_addr, size);
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index cf7e431cbc73..3279b5acea11 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -891,6 +891,7 @@ extern struct efi_memory_map memmap;
 
 extern int efi_reboot_quirk_mode;
 extern bool efi_poweroff_required(void);
+extern void __iomem *efi_remap(phys_addr_t phys_addr, size_t size);
 
 /* Iterate through an efi_memory_map */
 #define for_each_efi_memory_desc(m, md)					   \
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] efi: arch, x86: arch, ia64: rearrange EFI memmap related functions

[Matt Fleming]

On Mon, 04 May, at 02:02:14PM, Jonathan (Zhixiong) Zhang wrote:
> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
> 
> Both x86 and ia64 implemented efi_mem_attributs function, which is architecture
> agnositc. This function is moved to efi subsystem.
> 
> efi_remap() function is added. If EFI memmap feature is enabled, and if a
> memory region has attribute of EFI_MEMORY_UC, map it as uncached.
> 
> ---
> This patch was tested on an arm64 platform. It was built on x86 platform.
> 
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> ---
>  arch/ia64/kernel/efi.c      | 11 -----------
>  arch/x86/platform/efi/efi.c | 18 ------------------
>  drivers/firmware/efi/efi.c  | 27 +++++++++++++++++++++++++++
>  include/linux/efi.h         |  1 +
>  4 files changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/ia64/kernel/efi.c b/arch/ia64/kernel/efi.c
> index c52d7540dc05..ef20ec784b04 100644
> --- a/arch/ia64/kernel/efi.c
> +++ b/arch/ia64/kernel/efi.c
> @@ -771,17 +771,6 @@ efi_mem_type (unsigned long phys_addr)
>  }
>  
>  u64
> -efi_mem_attributes (unsigned long phys_addr)
> -{
> -	efi_memory_desc_t *md = efi_memory_descriptor(phys_addr);
> -
> -	if (md)
> -		return md->attribute;
> -	return 0;
> -}
> -EXPORT_SYMBOL(efi_mem_attributes);
> -
> -u64
>  efi_mem_attribute (unsigned long phys_addr, unsigned long size)
>  {
>  	unsigned long end = phys_addr + size;
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index dbc8627a5cdf..88b3ebaeb72f 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -917,24 +917,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"))

This should be split into two patches, one to remove the duplicate
efi_mem_attributes() and the other to create the new efi_ioremap()
function.

> +void __iomem *efi_remap(phys_addr_t phys_addr, size_t size)
> +{
> +	if (efi_enabled(EFI_MEMMAP) &&
> +	    (efi_mem_attributes(phys_addr) & EFI_MEMORY_UC))
> +		return ioremap(phys_addr, size);
> +	else
> +		return ioremap_cache(phys_addr, size);
> +}

Note that on x86 we don't leave the EFI memmap mapped throughout
runtime, it gets unmapped in efi_free_boot_services().

Which means that the second patch in this series isn't going to work
correctly if an error is reported after the kernel has finished booting.

It looks like arm64 leaves the EFI memmap mapped at runtime, right?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 1/2] efi: arch, x86: arch, ia64: rearrange EFI memmap related functions

[Zhang, Jonathan Zhixiong]

Thank you Matt very much for the code review! Pls. see comments inline.

On 5/27/2015 5:31 AM, Matt Fleming wrote:
> On Mon, 04 May, at 02:02:14PM, Jonathan (Zhixiong) Zhang wrote:
>> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
>>
>> Both x86 and ia64 implemented efi_mem_attributs function, which is architecture
>> agnositc. This function is moved to efi subsystem.
>>
>> efi_remap() function is added. If EFI memmap feature is enabled, and if a
>> memory region has attribute of EFI_MEMORY_UC, map it as uncached.
>>
>> ---
>> This patch was tested on an arm64 platform. It was built on x86 platform.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> ---
>>   arch/ia64/kernel/efi.c      | 11 -----------
>>   arch/x86/platform/efi/efi.c | 18 ------------------
>>   drivers/firmware/efi/efi.c  | 27 +++++++++++++++++++++++++++
>>   include/linux/efi.h         |  1 +
>>   4 files changed, 28 insertions(+), 29 deletions(-)
>>
 >> <snipped>
>
> This should be split into two patches, one to remove the duplicate
> efi_mem_attributes() and the other to create the new efi_ioremap()
> function.
Makes sense, will do.

>
>> +void __iomem *efi_remap(phys_addr_t phys_addr, size_t size)
>> +{
>> +	if (efi_enabled(EFI_MEMMAP) &&
>> +	    (efi_mem_attributes(phys_addr) & EFI_MEMORY_UC))
>> +		return ioremap(phys_addr, size);
>> +	else
>> +		return ioremap_cache(phys_addr, size);
>> +}
>
> Note that on x86 we don't leave the EFI memmap mapped throughout
> runtime, it gets unmapped in efi_free_boot_services().
>
> Which means that the second patch in this series isn't going to work
> correctly if an error is reported after the kernel has finished booting.
>
> It looks like arm64 leaves the EFI memmap mapped at runtime, right?
>
Correct, only x86 unmaps EFI memmap since efi_free_boot_services() is 
not implemented for other architectures, as shown in include/linux/efi.h .

With x86 arch, efi_unmap_memmap() is called in efi_free_boot_services() 
to unmap EFI memmap, this function is defined in 
arch/x86/platform/efi/efi.c. It clears EFI_MEMMAP bit in efi flag. 
Therefore in this case, efi_enabled(EFI_MEMMAP) returns false for x86 
platforms with EFI memory map support, after kernel finished booting; 
efi_remap() function in turn maps the memory region as cached.

Therefore, I believe the second patch in this series will work correctly 
for above situation as well. How do you think?

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

[PATCH 2/2] acpi, apei: use EFI memmap to map GHES memory

[Jonathan (Zhixiong) Zhang]

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

With ACPI APEI firmware first handling, generic hardware error
record is updated by firmware in GHES memory region. After firmware
updated GHES memory region, Linux reads stale data from cache.

GHES memory region should be mapped with cache attributes defined 
by EFI memory map when applicable. If firmware updates DDR directly, 
EFI memory map would have GHES memory region defined as uncached; 
If firmware updates cache, EFI memory map would have GHES memory 
region defined as cached.

When EFI is configued, map IRQ page using efi_remap() provided by
EFI subsystem.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
---
 drivers/acpi/apei/ghes.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index e82d0976a5d0..56875ca76aa7 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -48,6 +48,7 @@
 #include <linux/pci.h>
 #include <linux/aer.h>
 #include <linux/nmi.h>
+#include <linux/efi.h>
 
 #include <acpi/ghes.h>
 #include <acpi/apei.h>
@@ -159,6 +160,7 @@ static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)
 	return (void __iomem *)vaddr;
 }
 
+#ifndef CONFIG_EFI
 static void __iomem *ghes_ioremap_pfn_irq(u64 pfn)
 {
 	unsigned long vaddr;
@@ -169,6 +171,7 @@ static void __iomem *ghes_ioremap_pfn_irq(u64 pfn)
 
 	return (void __iomem *)vaddr;
 }
+#endif
 
 static void ghes_iounmap_nmi(void __iomem *vaddr_ptr)
 {
@@ -180,6 +183,7 @@ static void ghes_iounmap_nmi(void __iomem *vaddr_ptr)
 	arch_apei_flush_tlb_one(vaddr);
 }
 
+#ifndef CONFIG_EFI
 static void ghes_iounmap_irq(void __iomem *vaddr_ptr)
 {
 	unsigned long vaddr = (unsigned long __force)vaddr_ptr;
@@ -189,6 +193,7 @@ static void ghes_iounmap_irq(void __iomem *vaddr_ptr)
 	unmap_kernel_range_noflush(vaddr, PAGE_SIZE);
 	arch_apei_flush_tlb_one(vaddr);
 }
+#endif
 
 static int ghes_estatus_pool_init(void)
 {
@@ -309,7 +314,11 @@ static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len,
 			vaddr = ghes_ioremap_pfn_nmi(paddr >> PAGE_SHIFT);
 		} else {
 			spin_lock_irqsave(&ghes_ioremap_lock_irq, flags);
+#ifdef CONFIG_EFI
+			vaddr = efi_remap(paddr & PAGE_MASK, PAGE_SIZE);
+#else
 			vaddr = ghes_ioremap_pfn_irq(paddr >> PAGE_SHIFT);
+#endif
 		}
 		trunk = PAGE_SIZE - offset;
 		trunk = min(trunk, len);
@@ -324,7 +333,11 @@ static void ghes_copy_tofrom_phys(void *buffer, u64 paddr, u32 len,
 			ghes_iounmap_nmi(vaddr);
 			raw_spin_unlock(&ghes_ioremap_lock_nmi);
 		} else {
+#ifdef CONFIG_EFI
+			iounmap(vaddr);
+#else
 			ghes_iounmap_irq(vaddr);
+#endif
 			spin_unlock_irqrestore(&ghes_ioremap_lock_irq, flags);
 		}
 	}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project