[PATCH V2 0/3] map GHES memory region with EFI memory map

[PATCH V2 0/3] 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
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 arc/ia64 implemented architecture agnostic EFI
memory map attribue lookup function efi_memattributes(), the code is
moved from arch/x86 and arch/ia64 into EFI subsystem. On top of that,
efi_remap() is added.

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 (3):
  efi: arch, x86: arch, ia64: move efi_mem_attributes()
  efi: add efi_remap()
  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 V2 1/3] efi: arch, x86: arch, ia64: move efi_mem_attributes()

[Jonathan (Zhixiong) Zhang]

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

Both x86 and ia64 implemented efi_mem_attributs(), which is architecture
agnositc. This function is moved from arch/x86 and arch/ia64 to
drivers/firmware/efi.

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  | 18 ++++++++++++++++++
 3 files changed, 18 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 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..86da85368778 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 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 V2 2/3] efi: add efi_remap()

[Jonathan (Zhixiong) Zhang]

From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

If it is a EFI system, if EFI memmap is enabled and if
a memory region has attribute of EFI_MEMORY_UC, map it as uncached.

On x86, EFI memmap is unmapped in efi_free_boot_services(),
before kernel finishes booting. So when efi_remap() is called
during runtime on x86, it maps the region as cached.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/firmware/efi/efi.c | 9 +++++++++
 include/linux/efi.h        | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 86da85368778..5b42bb6d1fde 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -535,3 +535,12 @@ u64 efi_mem_attributes(unsigned long phys_addr)
 	}
 	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 af5be0368dec..0a0aa25d44d7 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -888,6 +888,7 @@ extern void efi_get_time(struct timespec *now);
 extern void efi_reserve_boot_services(void);
 extern int efi_get_fdt_params(struct efi_fdt_params *params, int verbose);
 extern struct efi_memory_map memmap;
+extern void __iomem *efi_remap(phys_addr_t phys_addr, size_t size);
 
 extern int efi_reboot_quirk_mode;
 extern bool efi_poweroff_required(void);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V2 1/3] efi: arch, x86: arch, ia64: move efi_mem_attributes()

[Matt Fleming]

(Cc'ing Tony for ia64 input)

On Mon, 01 Jun, at 12:12:18PM, Jonathan (Zhixiong) Zhang wrote:
> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> 
> Both x86 and ia64 implemented efi_mem_attributs(), which is architecture
> agnositc. This function is moved from arch/x86 and arch/ia64 to
> drivers/firmware/efi.
> 
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  arch/ia64/kernel/efi.c      | 11 -----------
>  arch/x86/platform/efi/efi.c | 18 ------------------
>  drivers/firmware/efi/efi.c  | 18 ++++++++++++++++++
>  3 files changed, 18 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 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..86da85368778 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 efi_mem_attributes(unsigned long phys_addr)
> +{
> +	efi_memory_desc_t *md;
> +	void *p;
> +
> +	if (!efi_enabled(EFI_MEMMAP))
> +		return 0;
> +

Umm... ia64 doesn't appear to set EFI_MEMMAP. So doesn't this change
actively break ia64?

While I like the idea of de-duplication, the two implementations of
efi_mem_attributes() are not equivalent.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V2 1/3] efi: arch, x86: arch, ia64: move efi_mem_attributes()

[Zhang, Jonathan Zhixiong]

Thank you for the feedback, Matt.

Given that IA64 does not set EFI_MEMMAP, it appears to me there
are following options:
A. Keep status quota and copy x86's efi_mem_attributes() code
to arm64.
B. In efi subsystem, provide week default efi_mem_attributes().
In the mean time, IA64 continues to have its own implementation.
C. Add EFI_MEMMAP support (and related bits) in IA64.

Which option do you prefer? Once there is a consensus, I am
willing to submit patch accordingly for review.

Regards,
Jonathan

On 6/2/2015 6:36 AM, Matt Fleming wrote:
> (Cc'ing Tony for ia64 input)
>
> On Mon, 01 Jun, at 12:12:18PM, Jonathan (Zhixiong) Zhang wrote:
>> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
>>
>> Both x86 and ia64 implemented efi_mem_attributs(), which is architecture
>> agnositc. This function is moved from arch/x86 and arch/ia64 to
>> drivers/firmware/efi.
>>
>> 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  | 18 ++++++++++++++++++
>>   3 files changed, 18 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 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..86da85368778 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 efi_mem_attributes(unsigned long phys_addr)
>> +{
>> +	efi_memory_desc_t *md;
>> +	void *p;
>> +
>> +	if (!efi_enabled(EFI_MEMMAP))
>> +		return 0;
>> +
>
> Umm... ia64 doesn't appear to set EFI_MEMMAP. So doesn't this change
> actively break ia64?
>
> While I like the idea of de-duplication, the two implementations of
> efi_mem_attributes() are not equivalent.
>

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

Re: [PATCH V2 1/3] efi: arch, x86: arch, ia64: move efi_mem_attributes()

[Matt Fleming]

On Tue, 02 Jun, at 05:09:14PM, Zhang, Jonathan Zhixiong wrote:
> Thank you for the feedback, Matt.
> 
> Given that IA64 does not set EFI_MEMMAP, it appears to me there
> are following options:
> A. Keep status quota and copy x86's efi_mem_attributes() code
> to arm64.

Let's avoid this option.

> B. In efi subsystem, provide week default efi_mem_attributes().
> In the mean time, IA64 continues to have its own implementation.

While I'm not a huge fan of using __weak this makes the most sense to me
because the alternative is to rename either the ia64 or x86
implementation and that just seems silly.

> C. Add EFI_MEMMAP support (and related bits) in IA64.

C. isn't an option because the ia64 memory map doesn't work the same way
as x86 and arm64.
 
> Which option do you prefer? Once there is a consensus, I am
> willing to submit patch accordingly for review.

Let's go with B. but please provide a comment above the weak
implementation explaining *why* it's declared as weak and that any new
architecture probably doesn't want to override it. Explain that the ia64
EFI memory map is special.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V2 1/3] efi: arch, x86: arch, ia64: move efi_mem_attributes()

[Zhang, Jonathan Zhixiong]

Sure, I will got with B with clear comment.

Thanks,
Jonathan

On 6/5/2015 2:23 AM, Matt Fleming wrote:
> On Tue, 02 Jun, at 05:09:14PM, Zhang, Jonathan Zhixiong wrote:
>> Thank you for the feedback, Matt.
>>
>> Given that IA64 does not set EFI_MEMMAP, it appears to me there
>> are following options:
>> A. Keep status quota and copy x86's efi_mem_attributes() code
>> to arm64.
>
> Let's avoid this option.
>
>> B. In efi subsystem, provide week default efi_mem_attributes().
>> In the mean time, IA64 continues to have its own implementation.
>
> While I'm not a huge fan of using __weak this makes the most sense to me
> because the alternative is to rename either the ia64 or x86
> implementation and that just seems silly.
>
>> C. Add EFI_MEMMAP support (and related bits) in IA64.
>
> C. isn't an option because the ia64 memory map doesn't work the same way
> as x86 and arm64.
>
>> Which option do you prefer? Once there is a consensus, I am
>> willing to submit patch accordingly for review.
>
> Let's go with B. but please provide a comment above the weak
> implementation explaining *why* it's declared as weak and that any new
> architecture probably doesn't want to override it. Explain that the ia64
> EFI memory map is special.
>

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