[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

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@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.

-- 
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

[PATCH V2 2/3] efi: add efi_remap()

[Jonathan (Zhixiong) Zhang]

From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.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@codeaurora.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

[PATCH V2 3/3] 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. When firmware
updated GHES memory region in DDR without going through cache,
Linux reads stale data from cache.

GHES memory region should be mapped with cache attributes
according to EFI memory map when applicable. If firmware updates
DDR directly, EFI memory map has GHES memory region defined as
uncached; If firmware updates cache, EFI memory map has 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

Re: [PATCH V2 3/3] acpi, apei: use EFI memmap to map GHES memory

[Matt Fleming]

[ Cc'ing Boris and Tony. Folks original patch is here,
  https://lkml.kernel.org/r/1433185940-24770-4-git-send-email-zjzhang@codeaurora.org ]

On Mon, 01 Jun, at 12:12:20PM, Jonathan (Zhixiong) Zhang wrote:
> 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. When firmware
> updated GHES memory region in DDR without going through cache,
> Linux reads stale data from cache.
> 
> GHES memory region should be mapped with cache attributes
> according to EFI memory map when applicable. If firmware updates
> DDR directly, EFI memory map has GHES memory region defined as
> uncached; If firmware updates cache, EFI memory map has GHES
> memory region defined as cached.
> 
> When EFI is configued, map IRQ page using efi_remap() provided by
> EFI subsystem.

[...]

> @@ -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;

Sprinkling CONFIG_EFI like this is wrong. On x86 we run kernels built
with CONFIG_EFI on machines with BIOS - you can't make the EFI vs.
non-EFI decision at compile-time.

So this patch looks like a potential regression to me since on x86
ghes_ioremap_pfn_irq() would not be used anymore and instead we'd be
using efi_remap() which will perform an ioremap_nocache() if it gets
called after efi_free_boot_services().

And based on the comments in the apei code, that's going to cause issues
because ioremap() does not work in atomic context, not to mention the
fact that we've gone from a cached mapping to an uncached one.

Instead, I suggest you modify ghes_ioremap_* to query the EFI memmap (if
it's available at runtime) to lookup the correct mapping attributes.

But I've Cc'd some more people who have actually worked on this code,
since I'm not one of them.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V2 3/3] acpi, apei: use EFI memmap to map GHES memory

[Borislav Petkov]

On Fri, Jun 05, 2015 at 10:57:01AM +0100, Matt Fleming wrote:
> [ Cc'ing Boris and Tony. Folks original patch is here,
>   https://lkml.kernel.org/r/1433185940-24770-4-git-send-email-zjzhang@codeaurora.org ]
> 
> On Mon, 01 Jun, at 12:12:20PM, Jonathan (Zhixiong) Zhang wrote:
> > 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. When firmware
> > updated GHES memory region in DDR without going through cache,

What is DDR?

I think this needs to be clarified first before we go any further.

I picked up on the sidelines that this might be arm64-specific stuff. If
so, your approach is wrong: you're merging efi_* facilities from x86 and
ia64 into generic efi ones but then doing CONFIG_EFI ifdeffery in GHES.

What you should do instead is have arch-specific:

ghes_ioremap_pfn_irq()
ghes_iounmap_irq()
...

and whatever else functionality which is different on your arch and
which get called from the generic ghes.c driver.

In the arch-specific ones you can go wild with the ifdeffery and whatnot
is needed on that specific arch.

Something like that, at least.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH V2 3/3] acpi, apei: use EFI memmap to map GHES memory

[Zhang, Jonathan Zhixiong]

Thank you Borislav for the review. Pls. see comments inline...

On 6/5/2015 3:25 AM, Borislav Petkov wrote:
> On Fri, Jun 05, 2015 at 10:57:01AM +0100, Matt Fleming wrote:
>> [ Cc'ing Boris and Tony. Folks original patch is here,
>>    https://lkml.kernel.org/r/1433185940-24770-4-git-send-email-zjzhang@codeaurora.org ]
>>
>> On Mon, 01 Jun, at 12:12:20PM, Jonathan (Zhixiong) Zhang wrote:
>>> 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. When firmware
>>> updated GHES memory region in DDR without going through cache,
>
> What is DDR?
>
> I think this needs to be clarified first before we go any further.
I thought the word "memory" might be confusing, because there are
memories on the system that is not accessible by Linux. In this
context, the APEI error data is accessed (read and write) by both Linux
and platform firmware; hence both sides should access the memory using
same cache attribute. I wanted to emphasize the idea that even though
normally DDR is cachable, but in this case when platform access it with
un-cached attribute, Linux should do the same.
I will try to make it more clear in next version of the patch.
>
> I picked up on the sidelines that this might be arm64-specific stuff. If
> so, your approach is wrong: you're merging efi_* facilities from x86 and
> ia64 into generic efi ones but then doing CONFIG_EFI ifdeffery in GHES.
>
> What you should do instead is have arch-specific:
>
> ghes_ioremap_pfn_irq()
> ghes_iounmap_irq()
> ...
>
> and whatever else functionality which is different on your arch and
> which get called from the generic ghes.c driver.
>
> In the arch-specific ones you can go wild with the ifdeffery and whatnot
> is needed on that specific arch.
>
> Something like that, at least.
Makes total sense. I was trying to reduce binary size for non-EFI
system, but as Matt pointed out in another feedback, on x86 even
BIOS based system has CONFIG_EFI enabled. I will submit a new version
accordingly.

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

Re: [PATCH V2 3/3] acpi, apei: use EFI memmap to map GHES memory

[Borislav Petkov]

On Fri, Jun 05, 2015 at 10:05:13AM -0700, Zhang, Jonathan Zhixiong wrote:
> >What is DDR?
> >
> >I think this needs to be clarified first before we go any further.
> I thought the word "memory" might be confusing, because there are

So you mean normal RAM here?

> memories on the system that is not accessible by Linux. In this
> context, the APEI error data is accessed (read and write) by both Linux
> and platform firmware; hence both sides should access the memory using
> same cache attribute. I wanted to emphasize the idea that even though
> normally DDR is cachable, but in this case when platform access it with
> un-cached attribute, Linux should do the same.

Makes sense.

Btw, do we need synchronization between firmware and Linux then? Does
Linux need to know when it is ok to touch that memory?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH V2 3/3] acpi, apei: use EFI memmap to map GHES memory

[Zhang, Jonathan Zhixiong]

On 6/5/2015 10:12 AM, Borislav Petkov wrote:
> On Fri, Jun 05, 2015 at 10:05:13AM -0700, Zhang, Jonathan Zhixiong wrote:
>>> What is DDR?
>>>
>>> I think this needs to be clarified first before we go any further.
>> I thought the word "memory" might be confusing, because there are
>
> So you mean normal RAM here?
Yes, exactly. I should use this word RAM instead.
>
>> memories on the system that is not accessible by Linux. In this
>> context, the APEI error data is accessed (read and write) by both Linux
>> and platform firmware; hence both sides should access the memory using
>> same cache attribute. I wanted to emphasize the idea that even though
>> normally DDR is cachable, but in this case when platform access it with
>> un-cached attribute, Linux should do the same.
>
> Makes sense.
>
> Btw, do we need synchronization between firmware and Linux then? Does
> Linux need to know when it is ok to touch that memory?
Good question. Linux zeros out error status code in the error data
after the data is consumed, this is good; but it alone does not solve
the synchronization concern.

For interrupt notification type (SCI or NMI) error source, this may not
be an issue since both sides can operate under the rule that the error
data is only overwritten but never appended. But what about poll
notification type? In this case, platform gathers error, updates the
memory region as needed; Linux checks the same memory region
periodically.

An ACPI APEI proposal intended to solve this concern has been discussed
in UEFI forum. The idea is to have OS to send platform a signal (through
updating a designated register) after error data is consumed. Therefore,
when OS is accessing the memory region, platform does not try to access
the same memory region in the mean time.

After this proposal is approved and published, I will submit a patch
to implement it.

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

Re: [PATCH V2 3/3] acpi, apei: use EFI memmap to map GHES memory

[Zhang, Jonathan Zhixiong]

Thanks Matt for the review. Yes, you are right on, I am following
this:
 > modify ghes_ioremap_* to query the EFI memmap (if
 > it's available at runtime) to lookup the correct mapping attributes.

Jonathan

On 6/5/2015 2:57 AM, Matt Fleming wrote:
> [ Cc'ing Boris and Tony. Folks original patch is here,
>    https://lkml.kernel.org/r/1433185940-24770-4-git-send-email-zjzhang@codeaurora.org ]
>
> On Mon, 01 Jun, at 12:12:20PM, Jonathan (Zhixiong) Zhang wrote:
>> 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. When firmware
>> updated GHES memory region in DDR without going through cache,
>> Linux reads stale data from cache.
>>
>> GHES memory region should be mapped with cache attributes
>> according to EFI memory map when applicable. If firmware updates
>> DDR directly, EFI memory map has GHES memory region defined as
>> uncached; If firmware updates cache, EFI memory map has GHES
>> memory region defined as cached.
>>
>> When EFI is configued, map IRQ page using efi_remap() provided by
>> EFI subsystem.
>
> [...]
>
>> @@ -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;
>
> Sprinkling CONFIG_EFI like this is wrong. On x86 we run kernels built
> with CONFIG_EFI on machines with BIOS - you can't make the EFI vs.
> non-EFI decision at compile-time.
>
> So this patch looks like a potential regression to me since on x86
> ghes_ioremap_pfn_irq() would not be used anymore and instead we'd be
> using efi_remap() which will perform an ioremap_nocache() if it gets
> called after efi_free_boot_services().
>
> And based on the comments in the apei code, that's going to cause issues
> because ioremap() does not work in atomic context, not to mention the
> fact that we've gone from a cached mapping to an uncached one.
>
> Instead, I suggest you modify ghes_ioremap_* to query the EFI memmap (if
> it's available at runtime) to lookup the correct mapping attributes.
>
> But I've Cc'd some more people who have actually worked on this code,
> since I'm not one of them.
>

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

Re: [PATCH V2 3/3] acpi, apei: use EFI memmap to map GHES memory

[Borislav Petkov]

On Fri, Jun 05, 2015 at 09:43:26AM -0700, Zhang, Jonathan Zhixiong wrote:
> Thanks Matt for the review. Yes, you are right on, I am following
> this:
> > modify ghes_ioremap_* to query the EFI memmap (if
> > it's available at runtime) to lookup the correct mapping attributes.

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

So please do not top-post.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH V2 3/3] acpi, apei: use EFI memmap to map GHES memory

[Zhang, Jonathan Zhixiong]

On 6/5/2015 9:50 AM, Borislav Petkov wrote:
> On Fri, Jun 05, 2015 at 09:43:26AM -0700, Zhang, Jonathan Zhixiong wrote:
>> Thanks Matt for the review. Yes, you are right on, I am following
>> this:
>>> modify ghes_ioremap_* to query the EFI memmap (if
>>> it's available at runtime) to lookup the correct mapping attributes.
>
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
>
> So please do not top-post.
>
> Thanks.
>
Will do. Thanks for the advise, Borislav.

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