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

[PATCH V5 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.

V5:
1. Rebased to next-20150713 of linux-next/master, efi-next-14359 of
   efi/next, pm+acpi-4.2-rc2 of linux-pm/master,
   arm64-fixes-1215 of arm64/master.
2. Added comment for efi_mem_attributes(), explained why it is marked
   as __weak at the function definition site.
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: implement 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/apei.c    | 11 +++++++++++
 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  | 31 +++++++++++++++++++++++++++++++
 include/acpi/apei.h         |  1 +
 6 files changed, 57 insertions(+), 20 deletions(-)

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

[PATCH V5 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  | 31 +++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 18 deletions(-)

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..bf4190a4f3f5 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -517,3 +517,34 @@ char * __init efi_md_typeattr_format(char *buf, size_t size,
 			 attr & EFI_MEMORY_UC      ? "UC"  : "");
 	return buf;
 }
+
+/*
+ * 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.
+ */
+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 V5 2/4] x86: acpi: implement 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.

Change-Id: Ia06d809c4dca05b68cbc9a94eea00c3f549a92e5
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 284801ac7042..64a12ce9880b 100644
--- a/include/acpi/apei.h
+++ b/include/acpi/apei.h
@@ -46,6 +46,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

[PATCH V5 3/4] arm64: apei: implement arch_apei_get_mem_attributes()

[Jonathan (Zhixiong) Zhang]

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

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

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
---
 arch/arm64/kernel/Makefile |  1 +
 arch/arm64/kernel/apei.c   | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)
 create mode 100644 arch/arm64/kernel/apei.c

diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 426d0763c81b..c135f1e4e3b8 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -36,6 +36,7 @@ arm64-obj-$(CONFIG_EFI)			+= efi.o efi-stub.o efi-entry.o
 arm64-obj-$(CONFIG_PCI)			+= pci.o
 arm64-obj-$(CONFIG_ARMV8_DEPRECATED)	+= armv8_deprecated.o
 arm64-obj-$(CONFIG_ACPI)		+= acpi.o
+arm64-obj-$(CONFIG_ACPI_APEI)		+= apei.o
 
 obj-y					+= $(arm64-obj-y) vdso/
 obj-m					+= $(arm64-obj-m)
diff --git a/arch/arm64/kernel/apei.c b/arch/arm64/kernel/apei.c
new file mode 100644
index 000000000000..9c6b62f40ad7
--- /dev/null
+++ b/arch/arm64/kernel/apei.c
@@ -0,0 +1,27 @@
+/*
+ * Arch-specific APEI-related functions.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <acpi/apei.h>
+
+#include <linux/efi.h>
+
+#include <asm/pgtable.h>
+
+pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
+{
+	if (efi_mem_attributes(addr) & EFI_MEMORY_UC)
+		return PROT_DEVICE_nGnRE;
+	else
+		return PAGE_KERNEL;
+}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH V5 4/4] acpi, apei: use appropriate pgprot_t 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 with uncached access attribute, Linux
reads stale data from cache.

GHES memory region should be mapped with page protection type
according to what is returned from arch_apei_get_mem_attribute(),
instead of always with PAGE_KERNEL (eg. cached attribute).

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

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index b979b5dbe5bc..98609b404dae 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -173,8 +173,10 @@ static void __iomem *ghes_ioremap_pfn_irq(u64 pfn)
 	unsigned long vaddr;
 
 	vaddr = (unsigned long)GHES_IOREMAP_IRQ_PAGE(ghes_ioremap_area->addr);
-	ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
-			   pfn << PAGE_SHIFT, PAGE_KERNEL);
+	ioremap_page_range(vaddr,
+			   vaddr + PAGE_SIZE,
+			   pfn << PAGE_SHIFT,
+			   arch_apei_get_mem_attribute(pfn << PAGE_SHIFT));
 
 	return (void __iomem *)vaddr;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V5 3/4] arm64: apei: implement arch_apei_get_mem_attributes()

[Will Deacon]

On Wed, Jul 15, 2015 at 01:31:55AM +0100, Jonathan (Zhixiong) Zhang wrote:
> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
> 
> If the physical address has memory attributes defined by EFI
> memmap as EFI_MEMORY_UC, the page protection type is
> PROT_DEVICE_nGnRE. Otherwise, the page protection type is
> PAGE_KERNEL.
> 
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> ---
>  arch/arm64/kernel/Makefile |  1 +
>  arch/arm64/kernel/apei.c   | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>  create mode 100644 arch/arm64/kernel/apei.c
> 
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 426d0763c81b..c135f1e4e3b8 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -36,6 +36,7 @@ arm64-obj-$(CONFIG_EFI)			+= efi.o efi-stub.o efi-entry.o
>  arm64-obj-$(CONFIG_PCI)			+= pci.o
>  arm64-obj-$(CONFIG_ARMV8_DEPRECATED)	+= armv8_deprecated.o
>  arm64-obj-$(CONFIG_ACPI)		+= acpi.o
> +arm64-obj-$(CONFIG_ACPI_APEI)		+= apei.o
>  
>  obj-y					+= $(arm64-obj-y) vdso/
>  obj-m					+= $(arm64-obj-m)
> diff --git a/arch/arm64/kernel/apei.c b/arch/arm64/kernel/apei.c
> new file mode 100644
> index 000000000000..9c6b62f40ad7
> --- /dev/null
> +++ b/arch/arm64/kernel/apei.c
> @@ -0,0 +1,27 @@
> +/*
> + * Arch-specific APEI-related functions.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <acpi/apei.h>
> +
> +#include <linux/efi.h>
> +
> +#include <asm/pgtable.h>
> +
> +pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
> +{
> +	if (efi_mem_attributes(addr) & EFI_MEMORY_UC)
> +		return PROT_DEVICE_nGnRE;
> +	else
> +		return PAGE_KERNEL;
> +}

Do we really need a new file and out-of-line call for this?

Will

Re: [PATCH V5 3/4] arm64: apei: implement arch_apei_get_mem_attributes()

[Zhang, Jonathan Zhixiong]

Thank you Will for the feedback. Pls. see comments below.

On 7/16/2015 10:18 AM, Will Deacon wrote:
> On Wed, Jul 15, 2015 at 01:31:55AM +0100, Jonathan (Zhixiong) Zhang wrote:
>> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
>>
>> If the physical address has memory attributes defined by EFI
>> memmap as EFI_MEMORY_UC, the page protection type is
>> PROT_DEVICE_nGnRE. Otherwise, the page protection type is
>> PAGE_KERNEL.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> ---
>>   arch/arm64/kernel/Makefile |  1 +
>>   arch/arm64/kernel/apei.c   | 27 +++++++++++++++++++++++++++
>>   2 files changed, 28 insertions(+)
>>   create mode 100644 arch/arm64/kernel/apei.c
>>
>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>> index 426d0763c81b..c135f1e4e3b8 100644
>> --- a/arch/arm64/kernel/Makefile
>> +++ b/arch/arm64/kernel/Makefile
>> @@ -36,6 +36,7 @@ arm64-obj-$(CONFIG_EFI)			+= efi.o efi-stub.o efi-entry.o
>>   arm64-obj-$(CONFIG_PCI)			+= pci.o
>>   arm64-obj-$(CONFIG_ARMV8_DEPRECATED)	+= armv8_deprecated.o
>>   arm64-obj-$(CONFIG_ACPI)		+= acpi.o
>> +arm64-obj-$(CONFIG_ACPI_APEI)		+= apei.o
>>
>>   obj-y					+= $(arm64-obj-y) vdso/
>>   obj-m					+= $(arm64-obj-m)
>> diff --git a/arch/arm64/kernel/apei.c b/arch/arm64/kernel/apei.c
>> new file mode 100644
>> index 000000000000..9c6b62f40ad7
>> --- /dev/null
>> +++ b/arch/arm64/kernel/apei.c
>> @@ -0,0 +1,27 @@
>> +/*
>> + * Arch-specific APEI-related functions.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <acpi/apei.h>
>> +
>> +#include <linux/efi.h>
>> +
>> +#include <asm/pgtable.h>
>> +
>> +pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>> +{
>> +	if (efi_mem_attributes(addr) & EFI_MEMORY_UC)
>> +		return PROT_DEVICE_nGnRE;
>> +	else
>> +		return PAGE_KERNEL;
>> +}
>
> Do we really need a new file and out-of-line call for this?
We have a choice of either adding this function to
arch/arm64/kernel/acpi.c, or creating
arch/arm64/kernel/apei.c. As we continue to work on firmware first
HW error handling for arm64, more arm64 specific APEI related functions
may need to be implemented, thus I think it would be good to create
arch/arm64/kernel/apei.c. That being said, to date we have found
the needs to have only two arm64 specific APEI related functions.
The other one can be found in LEG kernel, through this commit:
   aa2d69c88b27 ACPI, APEI, ARM64: APEI initial support for aarch64
My understanding is that Linaro will work on to upstream that commit. I
do not strongly prefer either choice.

When APEI ghes driver maps the memory region that has error record
updated by firmware, it executes in IRQ, timer or SEA handler. Since
ioremap() can not be used in atomic context, so APEI implements a
special version of atomic ioremap function calling ioremap_page_range().
On the other hand, x86 and ARM64 have different ways to define pgprot_t
for page that needs to be accessed with uncached property. x86 defines
PAGE_KERNEL_NOCACHE, while arm64 defines PROT_DEVICE_nGnRE. Therefore
arch specific implementation is needed.
There are other ways to achieve such needs. V3 of this
patch set tried another way [1]. I think the current way makes the most
sense, since it made generic APEI code to stay generic (no knowledge
about EFI, no arch dependent ifdefs).

There are discussions centering around making this memory attribute
retrieval generic. As Matt said in [2]: APEI case is special because
irrespective of what the kernel says we want to be compatible with the
firmware's memory map.

[1] 
http://fa.linux.kernel.narkive.com/YYXPJIjs/patch-v3-0-4-map-ghes-memory-region-with-efi-memory-map
[2] 
http://fa.linux.kernel.narkive.com/YYXPJIjs/patch-v3-0-4-map-ghes-memory-region-with-efi-memory-map


>
> Will
>

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

Re: [PATCH V5 3/4] arm64: apei: implement arch_apei_get_mem_attributes()

[Will Deacon]

On Fri, Jul 17, 2015 at 02:37:00AM +0100, Zhang, Jonathan Zhixiong wrote:
> On 7/16/2015 10:18 AM, Will Deacon wrote:
> > On Wed, Jul 15, 2015 at 01:31:55AM +0100, Jonathan (Zhixiong) Zhang wrote:
> >> +pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
> >> +{
> >> +	if (efi_mem_attributes(addr) & EFI_MEMORY_UC)
> >> +		return PROT_DEVICE_nGnRE;
> >> +	else
> >> +		return PAGE_KERNEL;
> >> +}
> >
> > Do we really need a new file and out-of-line call for this?
> We have a choice of either adding this function to
> arch/arm64/kernel/acpi.c, or creating
> arch/arm64/kernel/apei.c. As we continue to work on firmware first
> HW error handling for arm64, more arm64 specific APEI related functions
> may need to be implemented, thus I think it would be good to create
> arch/arm64/kernel/apei.c. That being said, to date we have found
> the needs to have only two arm64 specific APEI related functions.
> The other one can be found in LEG kernel, through this commit:
>    aa2d69c88b27 ACPI, APEI, ARM64: APEI initial support for aarch64
> My understanding is that Linaro will work on to upstream that commit. I
> do not strongly prefer either choice.
> 
> When APEI ghes driver maps the memory region that has error record
> updated by firmware, it executes in IRQ, timer or SEA handler. Since
> ioremap() can not be used in atomic context, so APEI implements a
> special version of atomic ioremap function calling ioremap_page_range().
> On the other hand, x86 and ARM64 have different ways to define pgprot_t
> for page that needs to be accessed with uncached property. x86 defines
> PAGE_KERNEL_NOCACHE, while arm64 defines PROT_DEVICE_nGnRE. Therefore
> arch specific implementation is needed.
> There are other ways to achieve such needs. V3 of this
> patch set tried another way [1]. I think the current way makes the most
> sense, since it made generic APEI code to stay generic (no knowledge
> about EFI, no arch dependent ifdefs).

I understand what you're doing and my concern was much simpler than you
seem to imagine. Put another way: why can't arch_apei_get_mem_attribute
be a static inline in a header file (like acpi_os_ioremap in asm/acpi.h)?

Will

Re: [PATCH V5 3/4] arm64: apei: implement arch_apei_get_mem_attributes()

[Hanjun Guo]

On 07/17/2015 09:37 AM, Zhang, Jonathan Zhixiong wrote:
> Thank you Will for the feedback. Pls. see comments below.
>
> On 7/16/2015 10:18 AM, Will Deacon wrote:
>> On Wed, Jul 15, 2015 at 01:31:55AM +0100, Jonathan (Zhixiong) Zhang
>> wrote:
>>> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
>>>
>>> If the physical address has memory attributes defined by EFI
>>> memmap as EFI_MEMORY_UC, the page protection type is
>>> PROT_DEVICE_nGnRE. Otherwise, the page protection type is
>>> PAGE_KERNEL.
>>>
>>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>>> ---
>>>   arch/arm64/kernel/Makefile |  1 +
>>>   arch/arm64/kernel/apei.c   | 27 +++++++++++++++++++++++++++
>>>   2 files changed, 28 insertions(+)
>>>   create mode 100644 arch/arm64/kernel/apei.c
>>>
>>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>>> index 426d0763c81b..c135f1e4e3b8 100644
>>> --- a/arch/arm64/kernel/Makefile
>>> +++ b/arch/arm64/kernel/Makefile
>>> @@ -36,6 +36,7 @@ arm64-obj-$(CONFIG_EFI)            += efi.o
>>> efi-stub.o efi-entry.o
>>>   arm64-obj-$(CONFIG_PCI)            += pci.o
>>>   arm64-obj-$(CONFIG_ARMV8_DEPRECATED)    += armv8_deprecated.o
>>>   arm64-obj-$(CONFIG_ACPI)        += acpi.o
>>> +arm64-obj-$(CONFIG_ACPI_APEI)        += apei.o
>>>
>>>   obj-y                    += $(arm64-obj-y) vdso/
>>>   obj-m                    += $(arm64-obj-m)
>>> diff --git a/arch/arm64/kernel/apei.c b/arch/arm64/kernel/apei.c
>>> new file mode 100644
>>> index 000000000000..9c6b62f40ad7
>>> --- /dev/null
>>> +++ b/arch/arm64/kernel/apei.c
>>> @@ -0,0 +1,27 @@
>>> +/*
>>> + * Arch-specific APEI-related functions.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <acpi/apei.h>
>>> +
>>> +#include <linux/efi.h>
>>> +
>>> +#include <asm/pgtable.h>
>>> +
>>> +pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>>> +{
>>> +    if (efi_mem_attributes(addr) & EFI_MEMORY_UC)
>>> +        return PROT_DEVICE_nGnRE;
>>> +    else
>>> +        return PAGE_KERNEL;
>>> +}
>>
>> Do we really need a new file and out-of-line call for this?
> We have a choice of either adding this function to
> arch/arm64/kernel/acpi.c, or creating
> arch/arm64/kernel/apei.c. As we continue to work on firmware first
> HW error handling for arm64, more arm64 specific APEI related functions
> may need to be implemented, thus I think it would be good to create
> arch/arm64/kernel/apei.c. That being said, to date we have found
> the needs to have only two arm64 specific APEI related functions.
> The other one can be found in LEG kernel, through this commit:
>    aa2d69c88b27 ACPI, APEI, ARM64: APEI initial support for aarch64
> My understanding is that Linaro will work on to upstream that commit. I
> do not strongly prefer either choice.

For me, I would like to locate those codes in arm64/kernel/acpi.c and
guard with CONFIG_ACPI_APEI now, if we want to extend in the future,
then let's see if we do anything for that, in this way, we can make ACPI
on ARM64 (the arch code) self-contained.

Thanks
Hanjun

Re: [PATCH V5 3/4] arm64: apei: implement arch_apei_get_mem_attributes()

[Zhang, Jonathan Zhixiong]

Thanks for the feedback, Hanjun.

On 7/17/2015 3:06 AM, Hanjun Guo wrote:
> On 07/17/2015 09:37 AM, Zhang, Jonathan Zhixiong wrote:
>> Thank you Will for the feedback. Pls. see comments below.
>>
>> On 7/16/2015 10:18 AM, Will Deacon wrote:
>>> On Wed, Jul 15, 2015 at 01:31:55AM +0100, Jonathan (Zhixiong) Zhang
>>> wrote:
>>>> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
>>>>
>>>> If the physical address has memory attributes defined by EFI
>>>> memmap as EFI_MEMORY_UC, the page protection type is
>>>> PROT_DEVICE_nGnRE. Otherwise, the page protection type is
>>>> PAGE_KERNEL.
>>>>
>>>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>>>> ---
>>>>   arch/arm64/kernel/Makefile |  1 +
>>>>   arch/arm64/kernel/apei.c   | 27 +++++++++++++++++++++++++++
>>>>   2 files changed, 28 insertions(+)
>>>>   create mode 100644 arch/arm64/kernel/apei.c
>>>>
>>>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>>>> index 426d0763c81b..c135f1e4e3b8 100644
>>>> --- a/arch/arm64/kernel/Makefile
>>>> +++ b/arch/arm64/kernel/Makefile
>>>> @@ -36,6 +36,7 @@ arm64-obj-$(CONFIG_EFI)            += efi.o
>>>> efi-stub.o efi-entry.o
>>>>   arm64-obj-$(CONFIG_PCI)            += pci.o
>>>>   arm64-obj-$(CONFIG_ARMV8_DEPRECATED)    += armv8_deprecated.o
>>>>   arm64-obj-$(CONFIG_ACPI)        += acpi.o
>>>> +arm64-obj-$(CONFIG_ACPI_APEI)        += apei.o
>>>>
>>>>   obj-y                    += $(arm64-obj-y) vdso/
>>>>   obj-m                    += $(arm64-obj-m)
>>>> diff --git a/arch/arm64/kernel/apei.c b/arch/arm64/kernel/apei.c
>>>> new file mode 100644
>>>> index 000000000000..9c6b62f40ad7
>>>> --- /dev/null
>>>> +++ b/arch/arm64/kernel/apei.c
>>>> @@ -0,0 +1,27 @@
>>>> +/*
>>>> + * Arch-specific APEI-related functions.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> modify
>>>> + * it under the terms of the GNU General Public License as
>>>> published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +
>>>> +#include <acpi/apei.h>
>>>> +
>>>> +#include <linux/efi.h>
>>>> +
>>>> +#include <asm/pgtable.h>
>>>> +
>>>> +pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>>>> +{
>>>> +    if (efi_mem_attributes(addr) & EFI_MEMORY_UC)
>>>> +        return PROT_DEVICE_nGnRE;
>>>> +    else
>>>> +        return PAGE_KERNEL;
>>>> +}
>>>
>>> Do we really need a new file and out-of-line call for this?
>> We have a choice of either adding this function to
>> arch/arm64/kernel/acpi.c, or creating
>> arch/arm64/kernel/apei.c. As we continue to work on firmware first
>> HW error handling for arm64, more arm64 specific APEI related functions
>> may need to be implemented, thus I think it would be good to create
>> arch/arm64/kernel/apei.c. That being said, to date we have found
>> the needs to have only two arm64 specific APEI related functions.
>> The other one can be found in LEG kernel, through this commit:
>>    aa2d69c88b27 ACPI, APEI, ARM64: APEI initial support for aarch64
>> My understanding is that Linaro will work on to upstream that commit. I
>> do not strongly prefer either choice.
>
> For me, I would like to locate those codes in arm64/kernel/acpi.c and
> guard with CONFIG_ACPI_APEI now, if we want to extend in the future,
> then let's see if we do anything for that, in this way, we can make ACPI
> on ARM64 (the arch code) self-contained.
Makes sense. Will do.
>
> Thanks
> Hanjun

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

Re: [PATCH V5 3/4] arm64: apei: implement arch_apei_get_mem_attributes()

[Zhang, Jonathan Zhixiong]

Thanks for the clarification, Will.

On 7/17/2015 2:43 AM, Will Deacon wrote:
> On Fri, Jul 17, 2015 at 02:37:00AM +0100, Zhang, Jonathan Zhixiong wrote:
>> On 7/16/2015 10:18 AM, Will Deacon wrote:
>>> On Wed, Jul 15, 2015 at 01:31:55AM +0100, Jonathan (Zhixiong) Zhang wrote:
>>>> +pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>>>> +{
>>>> +	if (efi_mem_attributes(addr) & EFI_MEMORY_UC)
>>>> +		return PROT_DEVICE_nGnRE;
>>>> +	else
>>>> +		return PAGE_KERNEL;
>>>> +}
>>>
>>> Do we really need a new file and out-of-line call for this?
>> We have a choice of either adding this function to
>> arch/arm64/kernel/acpi.c, or creating
>> arch/arm64/kernel/apei.c. As we continue to work on firmware first
>> HW error handling for arm64, more arm64 specific APEI related functions
>> may need to be implemented, thus I think it would be good to create
>> arch/arm64/kernel/apei.c. That being said, to date we have found
>> the needs to have only two arm64 specific APEI related functions.
>> The other one can be found in LEG kernel, through this commit:
>>     aa2d69c88b27 ACPI, APEI, ARM64: APEI initial support for aarch64
>> My understanding is that Linaro will work on to upstream that commit. I
>> do not strongly prefer either choice.
>>
>> When APEI ghes driver maps the memory region that has error record
>> updated by firmware, it executes in IRQ, timer or SEA handler. Since
>> ioremap() can not be used in atomic context, so APEI implements a
>> special version of atomic ioremap function calling ioremap_page_range().
>> On the other hand, x86 and ARM64 have different ways to define pgprot_t
>> for page that needs to be accessed with uncached property. x86 defines
>> PAGE_KERNEL_NOCACHE, while arm64 defines PROT_DEVICE_nGnRE. Therefore
>> arch specific implementation is needed.
>> There are other ways to achieve such needs. V3 of this
>> patch set tried another way [1]. I think the current way makes the most
>> sense, since it made generic APEI code to stay generic (no knowledge
>> about EFI, no arch dependent ifdefs).
>
> I understand what you're doing and my concern was much simpler than you
> seem to imagine. Put another way: why can't arch_apei_get_mem_attribute
> be a static inline in a header file (like acpi_os_ioremap in asm/acpi.h)?
Great. Will do.
>
> Will
>

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