[GIT PULL 0/2] EFI changes for v4.3 (part two)

[GIT PULL 0/2] EFI changes for v4.3 (part two)

[Matt Fleming]

From: Matt Fleming <matt.fleming@intel.com>

Folks, please pull the remaining parts of the EFI changes for v4.3.

Nothing much has changed in terms of code since the last version of
this part two pull request was sent on August 12th,

  https://lkml.kernel.org/r/1439396234-22863-1-git-send-email-matt@codeblueprint.co.uk

The main difference is the improved commit message in PATCH 2, which
now contains much more information as well as the user visible
side-effects of the issue that the patch fixes.

There were also some improvements to the comments inside PATCH 1 as
well as moving the function out of line at Ingo's suggestion.

The branch is based on tip/core/efi.

The following changes since commit 8d446c8647c9ab8fcb45a8fc7dbbafe1f83aa2f3:

  arm64/mm: Add PROT_DEVICE_nGnRnE and PROT_NORMAL_WT (2015-08-08 10:37:40 +0200)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git acpi/ghes

for you to fetch changes up to 404f03dbebede4d0ebeabd06e4b9120b481ae536:

  acpi, apei: Use appropriate pgprot_t to map GHES memory (2015-09-04 13:39:31 +0100)

----------------------------------------------------------------
Jonathan (Zhixiong) Zhang (2):
      arm64: apei: Implement arch_apei_get_mem_attributes()
      acpi, apei: Use appropriate pgprot_t to map GHES memory

 arch/arm64/include/asm/acpi.h |  5 +++++
 arch/arm64/kernel/acpi.c      | 29 +++++++++++++++++++++++++++++
 drivers/acpi/apei/ghes.c      | 10 +++++++---
 3 files changed, 41 insertions(+), 3 deletions(-)

[PATCH 1/2] arm64: apei: Implement arch_apei_get_mem_attributes()

[Matt Fleming]

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

Table 8 of UEFI 2.5 section 2.3.6.1 defines mappings from EFI
memory types to MAIR attribute encodings for arm64.

If the physical address has memory attributes defined by EFI
memmap as EFI_MEMORY_[UC|WC|WT], return approprate page protection
type according to the UEFI spec. Otherwise, return PAGE_KERNEL.

Reviewed-by: Matt Fleming <matt.fleming@intel.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 arch/arm64/include/asm/acpi.h |  5 +++++
 arch/arm64/kernel/acpi.c      | 29 +++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 406485ed110a..5aa892a12a0d 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -92,4 +92,9 @@ static inline const char *acpi_get_enable_method(int cpu)
 {
 	return acpi_psci_present() ? "psci" : NULL;
 }
+
+#ifdef	CONFIG_ACPI_APEI
+pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr);
+#endif
+
 #endif /*_ASM_ACPI_H*/
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 19de7537e7d3..9f083606e5bf 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -29,6 +29,11 @@
 #include <asm/cpu_ops.h>
 #include <asm/smp_plat.h>
 
+#ifdef CONFIG_ACPI_APEI
+#include <linux/efi.h>
+#include <asm/pgtable.h>
+#endif
+
 int acpi_noirq = 1;		/* skip ACPI IRQ initialization */
 int acpi_disabled = 1;
 EXPORT_SYMBOL(acpi_disabled);
@@ -230,3 +235,27 @@ void __init acpi_gic_init(void)
 
 	early_acpi_os_unmap_memory((char *)table, tbl_size);
 }
+
+#ifdef  CONFIG_ACPI_APEI
+pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
+{
+	/*
+	 * According to "Table 8 Map: EFI memory types to AArch64 memory
+	 * types" of UEFI 2.5 section 2.3.6.1, each EFI memory type is
+	 * mapped to a corresponding MAIR attribute encoding.
+	 * The EFI memory attribute advises all possible capabilities
+	 * of a memory region. We use the most efficient capability.
+	 */
+
+	u64 attr;
+
+	attr = efi_mem_attributes(addr);
+	if (attr & EFI_MEMORY_WB)
+		return PAGE_KERNEL;
+	if (attr & EFI_MEMORY_WT)
+		return __pgprot(PROT_NORMAL_WT);
+	if (attr & EFI_MEMORY_WC)
+		return __pgprot(PROT_NORMAL_NC);
+	return __pgprot(PROT_DEVICE_nGnRnE);
+}
+#endif
-- 
2.1.0

[PATCH 2/2] acpi, apei: Use appropriate pgprot_t to map GHES memory

[Matt Fleming]

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

If the ACPI APEI firmware handles hardware error first (called "firmware
first handling"), the firmware updates the GHES memory region with hardware
error record (called "generic hardware error record"). Essentially the
firmware writes hardware error records in the GHES memory region, triggers
an NMI/interrupt, then the GHES driver goes off and grabs the error record
from the GHES region.

The kernel currently maps the GHES memory region as cacheable
(PAGE_KERNEL) for all architectures. However, on some arm64 platforms,
there is a mismatch between how the kernel maps the GHES region
(PAGE_KERNEL) and how the firmware maps it (EFI_MEMORY_UC, ie.
uncacheable), leading to the possibility of the kernel GHES driver
reading stale data from the cache when it receives the interrupt.

With stale data being read, the kernel is unaware there is new hardware
error to be handled when there actually is; this may lead to further damage
in various scenarios, such as error propagation caused data corruption.
If uncorrected error (such as double bit ECC error) happened in memory
operation and if the kernel is unaware of such event happening, errorneous
data may be propagated to the disk.

Instead GHES memory region should be mapped with page protection type
according to what is returned from arch_apei_get_mem_attribute().

Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
Acked-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 drivers/acpi/apei/ghes.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 2bfd53cbfe80..e661695cf123 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -161,11 +161,15 @@ static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)
 
 static void __iomem *ghes_ioremap_pfn_irq(u64 pfn)
 {
-	unsigned long vaddr;
+	unsigned long vaddr, paddr;
+	pgprot_t prot;
 
 	vaddr = (unsigned long)GHES_IOREMAP_IRQ_PAGE(ghes_ioremap_area->addr);
-	ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
-			   pfn << PAGE_SHIFT, PAGE_KERNEL);
+
+	paddr = pfn << PAGE_SHIFT;
+	prot = arch_apei_get_mem_attribute(paddr);
+
+	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);
 
 	return (void __iomem *)vaddr;
 }
-- 
2.1.0

[PATCH 2/2] acpi, apei: Use appropriate pgprot_t to map GHES memory

[Matt Fleming]

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

If the ACPI APEI firmware handles hardware error first (called "firmware
first handling"), the firmware updates the GHES memory region with hardware
error record (called "generic hardware error record"). Essentially the
firmware writes hardware error records in the GHES memory region, triggers
an NMI/interrupt, then the GHES driver goes off and grabs the error record
from the GHES region.

The kernel currently maps the GHES memory region as cacheable
(PAGE_KERNEL) for all architectures. However, on some arm64 platforms,
there is a mismatch between how the kernel maps the GHES region
(PAGE_KERNEL) and how the firmware maps it (EFI_MEMORY_UC, ie.
uncacheable), leading to the possibility of the kernel GHES driver
reading stale data from the cache when it receives the interrupt.

With stale data being read, the kernel is unaware there is new hardware
error to be handled when there actually is; this may lead to further damage
in various scenarios, such as error propagation caused data corruption.
If uncorrected error (such as double bit ECC error) happened in memory
operation and if the kernel is unaware of such event happening, errorneous
data may be propagated to the disk.

Instead GHES memory region should be mapped with page protection type
according to what is returned from arch_apei_get_mem_attribute().

Reviewed-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/acpi/apei/ghes.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 2bfd53cbfe80..e661695cf123 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -161,11 +161,15 @@ static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)
 
 static void __iomem *ghes_ioremap_pfn_irq(u64 pfn)
 {
-	unsigned long vaddr;
+	unsigned long vaddr, paddr;
+	pgprot_t prot;
 
 	vaddr = (unsigned long)GHES_IOREMAP_IRQ_PAGE(ghes_ioremap_area->addr);
-	ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
-			   pfn << PAGE_SHIFT, PAGE_KERNEL);
+
+	paddr = pfn << PAGE_SHIFT;
+	prot = arch_apei_get_mem_attribute(paddr);
+
+	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);
 
 	return (void __iomem *)vaddr;
 }
-- 
2.1.0

Re: [GIT PULL 0/2] EFI changes for v4.3 (part two)

[Ingo Molnar]

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> From: Matt Fleming <matt.fleming@intel.com>
> 
> Folks, please pull the remaining parts of the EFI changes for v4.3.
> 
> Nothing much has changed in terms of code since the last version of
> this part two pull request was sent on August 12th,
> 
>   https://lkml.kernel.org/r/1439396234-22863-1-git-send-email-matt@codeblueprint.co.uk
> 
> The main difference is the improved commit message in PATCH 2, which
> now contains much more information as well as the user visible
> side-effects of the issue that the patch fixes.
> 
> There were also some improvements to the comments inside PATCH 1 as
> well as moving the function out of line at Ingo's suggestion.
> 
> The branch is based on tip/core/efi.
> 
> The following changes since commit 8d446c8647c9ab8fcb45a8fc7dbbafe1f83aa2f3:
> 
>   arm64/mm: Add PROT_DEVICE_nGnRnE and PROT_NORMAL_WT (2015-08-08 10:37:40 +0200)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git acpi/ghes
> 
> for you to fetch changes up to 404f03dbebede4d0ebeabd06e4b9120b481ae536:
> 
>   acpi, apei: Use appropriate pgprot_t to map GHES memory (2015-09-04 13:39:31 +0100)
> 
> ----------------------------------------------------------------
> Jonathan (Zhixiong) Zhang (2):
>       arm64: apei: Implement arch_apei_get_mem_attributes()
>       acpi, apei: Use appropriate pgprot_t to map GHES memory
> 
>  arch/arm64/include/asm/acpi.h |  5 +++++
>  arch/arm64/kernel/acpi.c      | 29 +++++++++++++++++++++++++++++
>  drivers/acpi/apei/ghes.c      | 10 +++++++---
>  3 files changed, 41 insertions(+), 3 deletions(-)

Applied to tip:core/efi, thanks Matt!

	Ingo

Re: [GIT PULL 0/2] EFI changes for v4.3 (part two)

[Ingo Molnar]

* Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:

> From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Folks, please pull the remaining parts of the EFI changes for v4.3.
> 
> Nothing much has changed in terms of code since the last version of
> this part two pull request was sent on August 12th,
> 
>   https://lkml.kernel.org/r/1439396234-22863-1-git-send-email-matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org
> 
> The main difference is the improved commit message in PATCH 2, which
> now contains much more information as well as the user visible
> side-effects of the issue that the patch fixes.
> 
> There were also some improvements to the comments inside PATCH 1 as
> well as moving the function out of line at Ingo's suggestion.
> 
> The branch is based on tip/core/efi.
> 
> The following changes since commit 8d446c8647c9ab8fcb45a8fc7dbbafe1f83aa2f3:
> 
>   arm64/mm: Add PROT_DEVICE_nGnRnE and PROT_NORMAL_WT (2015-08-08 10:37:40 +0200)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git acpi/ghes
> 
> for you to fetch changes up to 404f03dbebede4d0ebeabd06e4b9120b481ae536:
> 
>   acpi, apei: Use appropriate pgprot_t to map GHES memory (2015-09-04 13:39:31 +0100)
> 
> ----------------------------------------------------------------
> Jonathan (Zhixiong) Zhang (2):
>       arm64: apei: Implement arch_apei_get_mem_attributes()
>       acpi, apei: Use appropriate pgprot_t to map GHES memory
> 
>  arch/arm64/include/asm/acpi.h |  5 +++++
>  arch/arm64/kernel/acpi.c      | 29 +++++++++++++++++++++++++++++
>  drivers/acpi/apei/ghes.c      | 10 +++++++---
>  3 files changed, 41 insertions(+), 3 deletions(-)

Applied to tip:core/efi, thanks Matt!

	Ingo

[tip:core/efi] arm64, acpi/apei: Implement arch_apei_get_mem_attributes()

[tip-bot for Jonathan (Zhixiong) Zhang]

Commit-ID:  89e44b51cc0db50ea4b5bbb5d582c4db88bbaed8
Gitweb:     http://git.kernel.org/tip/89e44b51cc0db50ea4b5bbb5d582c4db88bbaed8
Author:     Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
AuthorDate: Fri, 4 Sep 2015 14:11:41 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 14 Sep 2015 11:40:03 +0200

arm64, acpi/apei: Implement arch_apei_get_mem_attributes()

Table 8 of UEFI 2.5 section 2.3.6.1 defines mappings from EFI
memory types to MAIR attribute encodings for arm64.

If the physical address has memory attributes defined by EFI
memmap as EFI_MEMORY_[UC|WC|WT], return approprate page
protection type according to the UEFI spec. Otherwise, return
PAGE_KERNEL.

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
[ Small stylistic tweaks. ]
Reviewed-by: Matt Fleming <matt.fleming@intel.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Link: http://lkml.kernel.org/r/1441372302-23242-2-git-send-email-matt@codeblueprint.co.uk
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/arm64/include/asm/acpi.h |  5 +++++
 arch/arm64/kernel/acpi.c      | 29 +++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 406485e..5aa892a 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -92,4 +92,9 @@ static inline const char *acpi_get_enable_method(int cpu)
 {
 	return acpi_psci_present() ? "psci" : NULL;
 }
+
+#ifdef	CONFIG_ACPI_APEI
+pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr);
+#endif
+
 #endif /*_ASM_ACPI_H*/
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 19de753..137d537 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -29,6 +29,11 @@
 #include <asm/cpu_ops.h>
 #include <asm/smp_plat.h>
 
+#ifdef CONFIG_ACPI_APEI
+# include <linux/efi.h>
+# include <asm/pgtable.h>
+#endif
+
 int acpi_noirq = 1;		/* skip ACPI IRQ initialization */
 int acpi_disabled = 1;
 EXPORT_SYMBOL(acpi_disabled);
@@ -230,3 +235,27 @@ void __init acpi_gic_init(void)
 
 	early_acpi_os_unmap_memory((char *)table, tbl_size);
 }
+
+#ifdef CONFIG_ACPI_APEI
+pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
+{
+	/*
+	 * According to "Table 8 Map: EFI memory types to AArch64 memory
+	 * types" of UEFI 2.5 section 2.3.6.1, each EFI memory type is
+	 * mapped to a corresponding MAIR attribute encoding.
+	 * The EFI memory attribute advises all possible capabilities
+	 * of a memory region. We use the most efficient capability.
+	 */
+
+	u64 attr;
+
+	attr = efi_mem_attributes(addr);
+	if (attr & EFI_MEMORY_WB)
+		return PAGE_KERNEL;
+	if (attr & EFI_MEMORY_WT)
+		return __pgprot(PROT_NORMAL_WT);
+	if (attr & EFI_MEMORY_WC)
+		return __pgprot(PROT_NORMAL_NC);
+	return __pgprot(PROT_DEVICE_nGnRnE);
+}
+#endif

[tip:core/efi] acpi/apei: Use appropriate pgprot_t to map GHES memory

[tip-bot for Jonathan (Zhixiong) Zhang]

Commit-ID:  8ece249a811e93d3f60e3f1ebdc86c7e7a95bdbf
Gitweb:     http://git.kernel.org/tip/8ece249a811e93d3f60e3f1ebdc86c7e7a95bdbf
Author:     Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
AuthorDate: Fri, 4 Sep 2015 14:11:42 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 14 Sep 2015 11:40:04 +0200

acpi/apei: Use appropriate pgprot_t to map GHES memory

If the ACPI APEI firmware handles hardware error first (called
"firmware first handling"), the firmware updates the GHES memory
region with hardware error record (called "generic hardware
error record"). Essentially the firmware writes hardware error
records in the GHES memory region, triggers an NMI/interrupt,
then the GHES driver goes off and grabs the error record from
the GHES region.

The kernel currently maps the GHES memory region as cacheable
(PAGE_KERNEL) for all architectures. However, on some arm64
platforms, there is a mismatch between how the kernel maps the
GHES region (PAGE_KERNEL) and how the firmware maps it
(EFI_MEMORY_UC, ie. uncacheable), leading to the possibility of
the kernel GHES driver reading stale data from the cache when it
receives the interrupt.

With stale data being read, the kernel is unaware there is new
hardware error to be handled when there actually is; this may
lead to further damage in various scenarios, such as error
propagation caused data corruption. If uncorrected error (such
as double bit ECC error) happened in memory operation and if the
kernel is unaware of such an event happening, errorneous data may
be propagated to the disk.

Instead GHES memory region should be mapped with page protection
type according to what is returned from arch_apei_get_mem_attribute().

Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
[ Small stylistic tweaks. ]
Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
Acked-by: Borislav Petkov <bp@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/1441372302-23242-3-git-send-email-matt@codeblueprint.co.uk
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/acpi/apei/ghes.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 2bfd53c..e661695 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -161,11 +161,15 @@ static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)
 
 static void __iomem *ghes_ioremap_pfn_irq(u64 pfn)
 {
-	unsigned long vaddr;
+	unsigned long vaddr, paddr;
+	pgprot_t prot;
 
 	vaddr = (unsigned long)GHES_IOREMAP_IRQ_PAGE(ghes_ioremap_area->addr);
-	ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
-			   pfn << PAGE_SHIFT, PAGE_KERNEL);
+
+	paddr = pfn << PAGE_SHIFT;
+	prot = arch_apei_get_mem_attribute(paddr);
+
+	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);
 
 	return (void __iomem *)vaddr;
 }

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Ingo Molnar]

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> On Tue, 25 Aug, at 10:27:22AM, Jonathan (Zhixiong) Zhang wrote:
> > From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
> > 
> > If the ACPI APEI firmware handles hardware error first (called "firmware
> > first handling"), the firmware updates the GHES memory region with hardware
> > error record (called "generic hardware error record"). Essentially the
> > firmware writes hardware error records in the GHES memory region, triggers
> > an NMI/interrupt, then the GHES driver goes off and grabs the error record
> > from the GHES region.
> > 
> > The kernel currently maps the GHES memory region as cacheable
> > (PAGE_KERNEL) for all architectures. However, on some arm64 platforms,
> > there is a mismatch between how the kernel maps the GHES region
> > (PAGE_KERNEL) and how the firmware maps it (EFI_MEMORY_UC, ie.
> > uncacheable), leading to the possibility of the kernel GHES driver
> > reading stale data from the cache when it receives the interrupt.
> > 
> > With stale data being read, the kernel is unaware there is new hardware
> > error to be handled when there actually is; this may lead to further damage
> > in various scenarios, such as error propagation caused data corruption.
> > If uncorrected error (such as double bit ECC error) happened in memory
> > operation and if the kernel is unaware of such event happening, errorneous
> > data may be propagated to the disk.
> > 
> > Instead GHES memory region should be mapped with page protection type
> > according to what is returned from arch_apei_get_mem_attribute().
> > 
> > Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
> > Acked-by: Borislav Petkov <bp@suse.de>
> > Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> > ---
> >  drivers/acpi/apei/ghes.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> This patch message looks fine to me. Ingo?

Looks good to me too!

Thanks,

	Ingo

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Ingo Molnar]

* Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:

> On Tue, 25 Aug, at 10:27:22AM, Jonathan (Zhixiong) Zhang wrote:
> > From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> > 
> > If the ACPI APEI firmware handles hardware error first (called "firmware
> > first handling"), the firmware updates the GHES memory region with hardware
> > error record (called "generic hardware error record"). Essentially the
> > firmware writes hardware error records in the GHES memory region, triggers
> > an NMI/interrupt, then the GHES driver goes off and grabs the error record
> > from the GHES region.
> > 
> > The kernel currently maps the GHES memory region as cacheable
> > (PAGE_KERNEL) for all architectures. However, on some arm64 platforms,
> > there is a mismatch between how the kernel maps the GHES region
> > (PAGE_KERNEL) and how the firmware maps it (EFI_MEMORY_UC, ie.
> > uncacheable), leading to the possibility of the kernel GHES driver
> > reading stale data from the cache when it receives the interrupt.
> > 
> > With stale data being read, the kernel is unaware there is new hardware
> > error to be handled when there actually is; this may lead to further damage
> > in various scenarios, such as error propagation caused data corruption.
> > If uncorrected error (such as double bit ECC error) happened in memory
> > operation and if the kernel is unaware of such event happening, errorneous
> > data may be propagated to the disk.
> > 
> > Instead GHES memory region should be mapped with page protection type
> > according to what is returned from arch_apei_get_mem_attribute().
> > 
> > Reviewed-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> > Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
> > Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> > ---
> >  drivers/acpi/apei/ghes.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> This patch message looks fine to me. Ingo?

Looks good to me too!

Thanks,

	Ingo

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Matt Fleming]

On Tue, 25 Aug, at 10:27:22AM, Jonathan (Zhixiong) Zhang wrote:
> From: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>
> 
> If the ACPI APEI firmware handles hardware error first (called "firmware
> first handling"), the firmware updates the GHES memory region with hardware
> error record (called "generic hardware error record"). Essentially the
> firmware writes hardware error records in the GHES memory region, triggers
> an NMI/interrupt, then the GHES driver goes off and grabs the error record
> from the GHES region.
> 
> The kernel currently maps the GHES memory region as cacheable
> (PAGE_KERNEL) for all architectures. However, on some arm64 platforms,
> there is a mismatch between how the kernel maps the GHES region
> (PAGE_KERNEL) and how the firmware maps it (EFI_MEMORY_UC, ie.
> uncacheable), leading to the possibility of the kernel GHES driver
> reading stale data from the cache when it receives the interrupt.
> 
> With stale data being read, the kernel is unaware there is new hardware
> error to be handled when there actually is; this may lead to further damage
> in various scenarios, such as error propagation caused data corruption.
> If uncorrected error (such as double bit ECC error) happened in memory
> operation and if the kernel is unaware of such event happening, errorneous
> data may be propagated to the disk.
> 
> Instead GHES memory region should be mapped with page protection type
> according to what is returned from arch_apei_get_mem_attribute().
> 
> Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
> Acked-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> ---
>  drivers/acpi/apei/ghes.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

This patch message looks fine to me. Ingo?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Matt Fleming]

On Tue, 25 Aug, at 10:27:22AM, Jonathan (Zhixiong) Zhang wrote:
> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> 
> If the ACPI APEI firmware handles hardware error first (called "firmware
> first handling"), the firmware updates the GHES memory region with hardware
> error record (called "generic hardware error record"). Essentially the
> firmware writes hardware error records in the GHES memory region, triggers
> an NMI/interrupt, then the GHES driver goes off and grabs the error record
> from the GHES region.
> 
> The kernel currently maps the GHES memory region as cacheable
> (PAGE_KERNEL) for all architectures. However, on some arm64 platforms,
> there is a mismatch between how the kernel maps the GHES region
> (PAGE_KERNEL) and how the firmware maps it (EFI_MEMORY_UC, ie.
> uncacheable), leading to the possibility of the kernel GHES driver
> reading stale data from the cache when it receives the interrupt.
> 
> With stale data being read, the kernel is unaware there is new hardware
> error to be handled when there actually is; this may lead to further damage
> in various scenarios, such as error propagation caused data corruption.
> If uncorrected error (such as double bit ECC error) happened in memory
> operation and if the kernel is unaware of such event happening, errorneous
> data may be propagated to the disk.
> 
> Instead GHES memory region should be mapped with page protection type
> according to what is returned from arch_apei_get_mem_attribute().
> 
> Reviewed-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  drivers/acpi/apei/ghes.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

This patch message looks fine to me. Ingo?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Zhang, Jonathan Zhixiong]

On 8/25/2015 1:59 AM, Ingo Molnar wrote:
>
> * Zhang, Jonathan Zhixiong <zjzhang@codeaurora.org> wrote:
>
>>
>>
>> On 8/22/2015 2:24 AM, Ingo Molnar wrote:
>>>
>>> * Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org> 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. On an arm64
>>>> platform, firmware updates GHES memory region with uncached
>>>> access attribute, and then Linux reads stale data from cache.
>>>
>>> This paragraph *still* doesn't parse for me. It's not any English
>>> I can recognize: what is a 'With ACPI APEI firmware first handling'?
>> APEI is ACPI Platform Error Interface; it is part of ACPI spec,
>> defining the aspect of hardware error handling. "firmware first
>> handling" is a terminology used in APEI. It describes such mechanism
>> that when hardware error happens, firmware intersects/handles such
>> hardware error, formulates hardware error record and writes the record
>> to GHES memory region, notifies the kernel through NMI/interrupt, then
>> the kernel GHES driver grabs the error record from the GHES memory
>> region.
>
> Argh. So how about translating that to English and putting that misnomer into
> scare quotes, and saying something like:
>
>    If the ACPI APEI firmware handles the error first (called "firmware first
>    handling"), the generic hardware error record is updated by the firmware in the
>    GHES memory region.
>
> ( Also note all the missing articles I added for readability. The rest of the
>    changelog is missing articles as well. )
Thank you very much, Ingo. Input are taken.
>
>>> ... plus what this changelog still doesn't mention is the most important part
>>> of any bug fix description: how does the user notice this in practice and why
>>> does he care?
>>
>> The changelog mentioned that Linux would read stale data from cache. When stale
>> data is read, kernel reports there is no new hardware error when there actually
>> is.
>
> Note that this is the most valuable sentence so far, in this whole changelog and
> discussion. And we needed how many emails to get to this point?
>
> obviously saying 'stale data' in itself does not mean much - it could mean a
> harmless inconsistency nobody really cares about, or in fact it could mean
> something more serious:
Sure, makes sense.
>
>> [...] This may lead to further damage in various scenarios, such as error
>> propagation caused data corruption.
>
> Please outline this better. How users are affected in practice is far more
> important than any other detail.
Yes, will do. I just sent out an update for your review.
>
> Thanks,
>
> 	Ingo
>

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

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Zhang, Jonathan Zhixiong]

On 8/25/2015 1:59 AM, Ingo Molnar wrote:
>
> * Zhang, Jonathan Zhixiong <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>
>>
>>
>> On 8/22/2015 2:24 AM, Ingo Molnar wrote:
>>>
>>> * Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>>>
>>>> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>>>
>>>> With ACPI APEI firmware first handling, generic hardware error
>>>> record is updated by firmware in GHES memory region. On an arm64
>>>> platform, firmware updates GHES memory region with uncached
>>>> access attribute, and then Linux reads stale data from cache.
>>>
>>> This paragraph *still* doesn't parse for me. It's not any English
>>> I can recognize: what is a 'With ACPI APEI firmware first handling'?
>> APEI is ACPI Platform Error Interface; it is part of ACPI spec,
>> defining the aspect of hardware error handling. "firmware first
>> handling" is a terminology used in APEI. It describes such mechanism
>> that when hardware error happens, firmware intersects/handles such
>> hardware error, formulates hardware error record and writes the record
>> to GHES memory region, notifies the kernel through NMI/interrupt, then
>> the kernel GHES driver grabs the error record from the GHES memory
>> region.
>
> Argh. So how about translating that to English and putting that misnomer into
> scare quotes, and saying something like:
>
>    If the ACPI APEI firmware handles the error first (called "firmware first
>    handling"), the generic hardware error record is updated by the firmware in the
>    GHES memory region.
>
> ( Also note all the missing articles I added for readability. The rest of the
>    changelog is missing articles as well. )
Thank you very much, Ingo. Input are taken.
>
>>> ... plus what this changelog still doesn't mention is the most important part
>>> of any bug fix description: how does the user notice this in practice and why
>>> does he care?
>>
>> The changelog mentioned that Linux would read stale data from cache. When stale
>> data is read, kernel reports there is no new hardware error when there actually
>> is.
>
> Note that this is the most valuable sentence so far, in this whole changelog and
> discussion. And we needed how many emails to get to this point?
>
> obviously saying 'stale data' in itself does not mean much - it could mean a
> harmless inconsistency nobody really cares about, or in fact it could mean
> something more serious:
Sure, makes sense.
>
>> [...] This may lead to further damage in various scenarios, such as error
>> propagation caused data corruption.
>
> Please outline this better. How users are affected in practice is far more
> important than any other detail.
Yes, will do. I just sent out an update for your review.
>
> Thanks,
>
> 	Ingo
>

-- 
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 appropriate pgprot_t to map GHES memory

[Jonathan (Zhixiong) Zhang]

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

If the ACPI APEI firmware handles hardware error first (called "firmware
first handling"), the firmware updates the GHES memory region with hardware
error record (called "generic hardware error record"). Essentially the
firmware writes hardware error records in the GHES memory region, triggers
an NMI/interrupt, then the GHES driver goes off and grabs the error record
from the GHES region.

The kernel currently maps the GHES memory region as cacheable
(PAGE_KERNEL) for all architectures. However, on some arm64 platforms,
there is a mismatch between how the kernel maps the GHES region
(PAGE_KERNEL) and how the firmware maps it (EFI_MEMORY_UC, ie.
uncacheable), leading to the possibility of the kernel GHES driver
reading stale data from the cache when it receives the interrupt.

With stale data being read, the kernel is unaware there is new hardware
error to be handled when there actually is; this may lead to further damage
in various scenarios, such as error propagation caused data corruption.
If uncorrected error (such as double bit ECC error) happened in memory
operation and if the kernel is unaware of such event happening, errorneous
data may be propagated to the disk.

Instead GHES memory region should be mapped with page protection type
according to what is returned from arch_apei_get_mem_attribute().

Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
Acked-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
---
 drivers/acpi/apei/ghes.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 23981ac1c6c2..3dd9c462d22a 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -157,11 +157,15 @@ static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)
 
 static void __iomem *ghes_ioremap_pfn_irq(u64 pfn)
 {
-	unsigned long vaddr;
+	unsigned long vaddr, paddr;
+	pgprot_t prot;
 
 	vaddr = (unsigned long)GHES_IOREMAP_IRQ_PAGE(ghes_ioremap_area->addr);
-	ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
-			   pfn << PAGE_SHIFT, PAGE_KERNEL);
+
+	paddr = pfn << PAGE_SHIFT;
+	prot = arch_apei_get_mem_attribute(paddr);
+
+	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);
 
 	return (void __iomem *)vaddr;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Jonathan (Zhixiong) Zhang]

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

If the ACPI APEI firmware handles hardware error first (called "firmware
first handling"), the firmware updates the GHES memory region with hardware
error record (called "generic hardware error record"). Essentially the
firmware writes hardware error records in the GHES memory region, triggers
an NMI/interrupt, then the GHES driver goes off and grabs the error record
from the GHES region.

The kernel currently maps the GHES memory region as cacheable
(PAGE_KERNEL) for all architectures. However, on some arm64 platforms,
there is a mismatch between how the kernel maps the GHES region
(PAGE_KERNEL) and how the firmware maps it (EFI_MEMORY_UC, ie.
uncacheable), leading to the possibility of the kernel GHES driver
reading stale data from the cache when it receives the interrupt.

With stale data being read, the kernel is unaware there is new hardware
error to be handled when there actually is; this may lead to further damage
in various scenarios, such as error propagation caused data corruption.
If uncorrected error (such as double bit ECC error) happened in memory
operation and if the kernel is unaware of such event happening, errorneous
data may be propagated to the disk.

Instead GHES memory region should be mapped with page protection type
according to what is returned from arch_apei_get_mem_attribute().

Reviewed-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/acpi/apei/ghes.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 23981ac1c6c2..3dd9c462d22a 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -157,11 +157,15 @@ static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)
 
 static void __iomem *ghes_ioremap_pfn_irq(u64 pfn)
 {
-	unsigned long vaddr;
+	unsigned long vaddr, paddr;
+	pgprot_t prot;
 
 	vaddr = (unsigned long)GHES_IOREMAP_IRQ_PAGE(ghes_ioremap_area->addr);
-	ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
-			   pfn << PAGE_SHIFT, PAGE_KERNEL);
+
+	paddr = pfn << PAGE_SHIFT;
+	prot = arch_apei_get_mem_attribute(paddr);
+
+	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);
 
 	return (void __iomem *)vaddr;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Ingo Molnar]

* Zhang, Jonathan Zhixiong <zjzhang@codeaurora.org> wrote:

> 
> 
> On 8/22/2015 2:24 AM, Ingo Molnar wrote:
> >
> >* Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org> 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. On an arm64
> >>platform, firmware updates GHES memory region with uncached
> >>access attribute, and then Linux reads stale data from cache.
> >
> >This paragraph *still* doesn't parse for me. It's not any English
> >I can recognize: what is a 'With ACPI APEI firmware first handling'?
> APEI is ACPI Platform Error Interface; it is part of ACPI spec,
> defining the aspect of hardware error handling. "firmware first
> handling" is a terminology used in APEI. It describes such mechanism
> that when hardware error happens, firmware intersects/handles such
> hardware error, formulates hardware error record and writes the record
> to GHES memory region, notifies the kernel through NMI/interrupt, then
> the kernel GHES driver grabs the error record from the GHES memory
> region.

Argh. So how about translating that to English and putting that misnomer into 
scare quotes, and saying something like:

  If the ACPI APEI firmware handles the error first (called "firmware first 
  handling"), the generic hardware error record is updated by the firmware in the 
  GHES memory region.

( Also note all the missing articles I added for readability. The rest of the 
  changelog is missing articles as well. )

> > ... plus what this changelog still doesn't mention is the most important part 
> > of any bug fix description: how does the user notice this in practice and why 
> > does he care?
>
> The changelog mentioned that Linux would read stale data from cache. When stale 
> data is read, kernel reports there is no new hardware error when there actually 
> is.

Note that this is the most valuable sentence so far, in this whole changelog and 
discussion. And we needed how many emails to get to this point?

obviously saying 'stale data' in itself does not mean much - it could mean a 
harmless inconsistency nobody really cares about, or in fact it could mean 
something more serious:

> [...] This may lead to further damage in various scenarios, such as error 
> propagation caused data corruption.

Please outline this better. How users are affected in practice is far more 
important than any other detail.

Thanks,

	Ingo

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Ingo Molnar]

* Zhang, Jonathan Zhixiong <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:

> 
> 
> On 8/22/2015 2:24 AM, Ingo Molnar wrote:
> >
> >* Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> >
> >>From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> >>
> >>With ACPI APEI firmware first handling, generic hardware error
> >>record is updated by firmware in GHES memory region. On an arm64
> >>platform, firmware updates GHES memory region with uncached
> >>access attribute, and then Linux reads stale data from cache.
> >
> >This paragraph *still* doesn't parse for me. It's not any English
> >I can recognize: what is a 'With ACPI APEI firmware first handling'?
> APEI is ACPI Platform Error Interface; it is part of ACPI spec,
> defining the aspect of hardware error handling. "firmware first
> handling" is a terminology used in APEI. It describes such mechanism
> that when hardware error happens, firmware intersects/handles such
> hardware error, formulates hardware error record and writes the record
> to GHES memory region, notifies the kernel through NMI/interrupt, then
> the kernel GHES driver grabs the error record from the GHES memory
> region.

Argh. So how about translating that to English and putting that misnomer into 
scare quotes, and saying something like:

  If the ACPI APEI firmware handles the error first (called "firmware first 
  handling"), the generic hardware error record is updated by the firmware in the 
  GHES memory region.

( Also note all the missing articles I added for readability. The rest of the 
  changelog is missing articles as well. )

> > ... plus what this changelog still doesn't mention is the most important part 
> > of any bug fix description: how does the user notice this in practice and why 
> > does he care?
>
> The changelog mentioned that Linux would read stale data from cache. When stale 
> data is read, kernel reports there is no new hardware error when there actually 
> is.

Note that this is the most valuable sentence so far, in this whole changelog and 
discussion. And we needed how many emails to get to this point?

obviously saying 'stale data' in itself does not mean much - it could mean a 
harmless inconsistency nobody really cares about, or in fact it could mean 
something more serious:

> [...] This may lead to further damage in various scenarios, such as error 
> propagation caused data corruption.

Please outline this better. How users are affected in practice is far more 
important than any other detail.

Thanks,

	Ingo

[PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Jonathan (Zhixiong) Zhang]

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

GHES memory region is used as a communication buffer for reporting
hardware errors from the firmware to kernel. Essentially the
firmware writes hardware error records there, triggers an NMI/interrupt,
then the GHES driver goes off and grabs the error record from the
GHES region.

The kernel currently maps the GHES memory region as cacheable
(PAGE_KERNEL) for all architectures. However, on some arm64 platforms,
there is a mismatch between how the kernel maps the GHES region
(PAGE_KERNEL) and how the firmware maps it (EFI_MEMORY_UC, ie.
uncacheable), leading to the possibility of the kernel GHES driver
reading stale data from the cache when it receives the interrupt. With
stale data being read, kernel reports there is no new hardware error
when there actually is; this may lead to further damage in various
scenarios, such as error propagation caused data corruption.

Instead GHES memory region should be mapped with page protection type
according to what is returned from arch_apei_get_mem_attribute().

Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>
Acked-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
---
 drivers/acpi/apei/ghes.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 23981ac1c6c2..3dd9c462d22a 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -157,11 +157,15 @@ static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)
 
 static void __iomem *ghes_ioremap_pfn_irq(u64 pfn)
 {
-	unsigned long vaddr;
+	unsigned long vaddr, paddr;
+	pgprot_t prot;
 
 	vaddr = (unsigned long)GHES_IOREMAP_IRQ_PAGE(ghes_ioremap_area->addr);
-	ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
-			   pfn << PAGE_SHIFT, PAGE_KERNEL);
+
+	paddr = pfn << PAGE_SHIFT;
+	prot = arch_apei_get_mem_attribute(paddr);
+
+	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);
 
 	return (void __iomem *)vaddr;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Jonathan (Zhixiong) Zhang]

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

GHES memory region is used as a communication buffer for reporting
hardware errors from the firmware to kernel. Essentially the
firmware writes hardware error records there, triggers an NMI/interrupt,
then the GHES driver goes off and grabs the error record from the
GHES region.

The kernel currently maps the GHES memory region as cacheable
(PAGE_KERNEL) for all architectures. However, on some arm64 platforms,
there is a mismatch between how the kernel maps the GHES region
(PAGE_KERNEL) and how the firmware maps it (EFI_MEMORY_UC, ie.
uncacheable), leading to the possibility of the kernel GHES driver
reading stale data from the cache when it receives the interrupt. With
stale data being read, kernel reports there is no new hardware error
when there actually is; this may lead to further damage in various
scenarios, such as error propagation caused data corruption.

Instead GHES memory region should be mapped with page protection type
according to what is returned from arch_apei_get_mem_attribute().

Reviewed-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/acpi/apei/ghes.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 23981ac1c6c2..3dd9c462d22a 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -157,11 +157,15 @@ static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)
 
 static void __iomem *ghes_ioremap_pfn_irq(u64 pfn)
 {
-	unsigned long vaddr;
+	unsigned long vaddr, paddr;
+	pgprot_t prot;
 
 	vaddr = (unsigned long)GHES_IOREMAP_IRQ_PAGE(ghes_ioremap_area->addr);
-	ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
-			   pfn << PAGE_SHIFT, PAGE_KERNEL);
+
+	paddr = pfn << PAGE_SHIFT;
+	prot = arch_apei_get_mem_attribute(paddr);
+
+	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);
 
 	return (void __iomem *)vaddr;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Zhang, Jonathan Zhixiong]

On 8/22/2015 2:24 AM, Ingo Molnar wrote:
>
> * Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org> 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. On an arm64
>> platform, firmware updates GHES memory region with uncached
>> access attribute, and then Linux reads stale data from cache.
>
> This paragraph *still* doesn't parse for me. It's not any English
> I can recognize: what is a 'With ACPI APEI firmware first handling'?
APEI is ACPI Platform Error Interface; it is part of ACPI spec,
defining the aspect of hardware error handling. "firmware first
handling" is a terminology used in APEI. It describes such mechanism
that when hardware error happens, firmware intersects/handles such
hardware error, formulates hardware error record and writes the record
to GHES memory region, notifies the kernel through NMI/interrupt, then
the kernel GHES driver grabs the error record from the GHES memory
region.

>
>> With current code, GHES memory region is mapped with PAGE_KERNEL
>> based on the assumption that cache coherency of GHES memory region
>> is maintained by firmware on all platforms. This assumption is
>> not true for above mentioned arm64 platform.
>>
>> Instead GHES memory region should be mapped with page protection type
>> according to what is returned from arch_apei_get_mem_attribute().
>
> ... plus what this changelog still doesn't mention is the most important part of
> any bug fix description: how does the user notice this in practice and why does he
> care?
The changelog mentioned that Linux would read stale data from cache.
When stale data is read, kernel reports there is no new hardware error
when there actually is. This may lead to further damage in various
scenarios, such as error propagation caused data corruption.
>
> Thanks,
>
> 	Ingo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Zhang, Jonathan Zhixiong]

On 8/22/2015 2:24 AM, Ingo Molnar wrote:
>
> * Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>
>> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>
>> With ACPI APEI firmware first handling, generic hardware error
>> record is updated by firmware in GHES memory region. On an arm64
>> platform, firmware updates GHES memory region with uncached
>> access attribute, and then Linux reads stale data from cache.
>
> This paragraph *still* doesn't parse for me. It's not any English
> I can recognize: what is a 'With ACPI APEI firmware first handling'?
APEI is ACPI Platform Error Interface; it is part of ACPI spec,
defining the aspect of hardware error handling. "firmware first
handling" is a terminology used in APEI. It describes such mechanism
that when hardware error happens, firmware intersects/handles such
hardware error, formulates hardware error record and writes the record
to GHES memory region, notifies the kernel through NMI/interrupt, then
the kernel GHES driver grabs the error record from the GHES memory
region.

>
>> With current code, GHES memory region is mapped with PAGE_KERNEL
>> based on the assumption that cache coherency of GHES memory region
>> is maintained by firmware on all platforms. This assumption is
>> not true for above mentioned arm64 platform.
>>
>> Instead GHES memory region should be mapped with page protection type
>> according to what is returned from arch_apei_get_mem_attribute().
>
> ... plus what this changelog still doesn't mention is the most important part of
> any bug fix description: how does the user notice this in practice and why does he
> care?
The changelog mentioned that Linux would read stale data from cache.
When stale data is read, kernel reports there is no new hardware error
when there actually is. This may lead to further damage in various
scenarios, such as error propagation caused data corruption.
>
> Thanks,
>
> 	Ingo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Ingo Molnar]

* Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org> 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. On an arm64
> platform, firmware updates GHES memory region with uncached
> access attribute, and then Linux reads stale data from cache.

This paragraph *still* doesn't parse for me. It's not any English
I can recognize: what is a 'With ACPI APEI firmware first handling'?

> With current code, GHES memory region is mapped with PAGE_KERNEL
> based on the assumption that cache coherency of GHES memory region
> is maintained by firmware on all platforms. This assumption is
> not true for above mentioned arm64 platform.
> 
> Instead GHES memory region should be mapped with page protection type
> according to what is returned from arch_apei_get_mem_attribute().

... plus what this changelog still doesn't mention is the most important part of 
any bug fix description: how does the user notice this in practice and why does he 
care?

Thanks,

	Ingo

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Ingo Molnar]

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

> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> 
> With ACPI APEI firmware first handling, generic hardware error
> record is updated by firmware in GHES memory region. On an arm64
> platform, firmware updates GHES memory region with uncached
> access attribute, and then Linux reads stale data from cache.

This paragraph *still* doesn't parse for me. It's not any English
I can recognize: what is a 'With ACPI APEI firmware first handling'?

> With current code, GHES memory region is mapped with PAGE_KERNEL
> based on the assumption that cache coherency of GHES memory region
> is maintained by firmware on all platforms. This assumption is
> not true for above mentioned arm64 platform.
> 
> Instead GHES memory region should be mapped with page protection type
> according to what is returned from arch_apei_get_mem_attribute().

... plus what this changelog still doesn't mention is the most important part of 
any bug fix description: how does the user notice this in practice and why does he 
care?

Thanks,

	Ingo

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Zhang, Jonathan Zhixiong]

On 8/17/2015 6:13 AM, Matt Fleming wrote:
> On Fri, 14 Aug, at 03:37:30PM, 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. On an arm64
>> platform, firmware updates GHES memory region with uncached
>> access attribute, and then Linux reads stale data from cache.
>>
>> With current code, GHES memory region is mapped with PAGE_KERNEL
>> based on the assumption that cache coherency of GHES memory region
>> is maintained by firmware on all platforms. This assumption is
>> not true for above mentioned arm64 platform.
>>
>> Instead GHES memory region should be mapped with page protection type
>> according to what is returned from arch_apei_get_mem_attribute().
>>
>> Acked-by: Borislav Petkov <bp@suse.de>
>> 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 23981ac1c6c2..f742335bd8bb 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -160,8 +160,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;
>>   }
>
> Y'know, this would be more readable if written as,
>
> 	unsigned long vaddr, paddr;
> 	pgprot_t prot;
>
>    	vaddr = (unsigned long)GHES_IOREMAP_IRQ_PAGE(ghes_ioremap_area->addr);
>
> 	paddr = pfn << PAGE_SHIFT;
> 	prot = arch_apei_get_mem_attribute(paddr);
>
> 	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);
>
> But either way,
>
> Reviewed-by: Matt Fleming <matt.fleming@intel.com>
Thanks. I will go with the easier to read way.
>

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

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Zhang, Jonathan Zhixiong]

On 8/17/2015 6:13 AM, Matt Fleming wrote:
> On Fri, 14 Aug, at 03:37:30PM, Jonathan (Zhixiong) Zhang wrote:
>> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>
>> With ACPI APEI firmware first handling, generic hardware error
>> record is updated by firmware in GHES memory region. On an arm64
>> platform, firmware updates GHES memory region with uncached
>> access attribute, and then Linux reads stale data from cache.
>>
>> With current code, GHES memory region is mapped with PAGE_KERNEL
>> based on the assumption that cache coherency of GHES memory region
>> is maintained by firmware on all platforms. This assumption is
>> not true for above mentioned arm64 platform.
>>
>> Instead GHES memory region should be mapped with page protection type
>> according to what is returned from arch_apei_get_mem_attribute().
>>
>> Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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 23981ac1c6c2..f742335bd8bb 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -160,8 +160,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;
>>   }
>
> Y'know, this would be more readable if written as,
>
> 	unsigned long vaddr, paddr;
> 	pgprot_t prot;
>
>    	vaddr = (unsigned long)GHES_IOREMAP_IRQ_PAGE(ghes_ioremap_area->addr);
>
> 	paddr = pfn << PAGE_SHIFT;
> 	prot = arch_apei_get_mem_attribute(paddr);
>
> 	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);
>
> But either way,
>
> Reviewed-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Thanks. I will go with the easier to read way.
>

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

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Matt Fleming]

On Fri, 14 Aug, at 03:37:30PM, 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. On an arm64
> platform, firmware updates GHES memory region with uncached
> access attribute, and then Linux reads stale data from cache.
> 
> With current code, GHES memory region is mapped with PAGE_KERNEL
> based on the assumption that cache coherency of GHES memory region
> is maintained by firmware on all platforms. This assumption is
> not true for above mentioned arm64 platform.
> 
> Instead GHES memory region should be mapped with page protection type
> according to what is returned from arch_apei_get_mem_attribute().
> 
> Acked-by: Borislav Petkov <bp@suse.de>
> 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 23981ac1c6c2..f742335bd8bb 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -160,8 +160,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;
>  }

Y'know, this would be more readable if written as,

	unsigned long vaddr, paddr;
	pgprot_t prot;

  	vaddr = (unsigned long)GHES_IOREMAP_IRQ_PAGE(ghes_ioremap_area->addr);

	paddr = pfn << PAGE_SHIFT;
	prot = arch_apei_get_mem_attribute(paddr);

	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);

But either way,

Reviewed-by: Matt Fleming <matt.fleming@intel.com>

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Matt Fleming]

On Fri, 14 Aug, at 03:37:30PM, Jonathan (Zhixiong) Zhang wrote:
> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> 
> With ACPI APEI firmware first handling, generic hardware error
> record is updated by firmware in GHES memory region. On an arm64
> platform, firmware updates GHES memory region with uncached
> access attribute, and then Linux reads stale data from cache.
> 
> With current code, GHES memory region is mapped with PAGE_KERNEL
> based on the assumption that cache coherency of GHES memory region
> is maintained by firmware on all platforms. This assumption is
> not true for above mentioned arm64 platform.
> 
> Instead GHES memory region should be mapped with page protection type
> according to what is returned from arch_apei_get_mem_attribute().
> 
> Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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 23981ac1c6c2..f742335bd8bb 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -160,8 +160,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;
>  }

Y'know, this would be more readable if written as,

	unsigned long vaddr, paddr;
	pgprot_t prot;

  	vaddr = (unsigned long)GHES_IOREMAP_IRQ_PAGE(ghes_ioremap_area->addr);

	paddr = pfn << PAGE_SHIFT;
	prot = arch_apei_get_mem_attribute(paddr);

	ioremap_page_range(vaddr, vaddr + PAGE_SIZE, paddr, prot);

But either way,

Reviewed-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH 2/2] 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. On an arm64
platform, firmware updates GHES memory region with uncached
access attribute, and then Linux reads stale data from cache.

With current code, GHES memory region is mapped with PAGE_KERNEL
based on the assumption that cache coherency of GHES memory region
is maintained by firmware on all platforms. This assumption is
not true for above mentioned arm64 platform.

Instead GHES memory region should be mapped with page protection type
according to what is returned from arch_apei_get_mem_attribute().

Acked-by: Borislav Petkov <bp@suse.de>
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 23981ac1c6c2..f742335bd8bb 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -160,8 +160,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 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Zhang, Jonathan Zhixiong]

On 8/13/2015 4:14 AM, Will Deacon wrote:
> On Thu, Aug 13, 2015 at 10:24:32AM +0100, Matt Fleming wrote:
>> On Thu, 13 Aug, at 10:19:17AM, Ingo Molnar wrote:
>>> * Matt Fleming <matt@codeblueprint.co.uk> 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. On an arm64
>>>> platform, firmware updates GHES memory region with uncached
>>>> access attribute, and then Linux reads stale data from cache.
>>>
>>> So this paragraph does not parse for me ...
I will update the paragraph to explain that with current code,
PAGE_KERNEL is always used, this means that the kernel assumes
cache coherency to the memory region is maintained by firmware;
such assumption is not always correct.
>>>
>>> If it tries to explain a bug it falls very short of doing a proper job of that.
>>
>> I'll let Jonathan provide more details but I understood the problem to
>> be a cache (in)coherency issue.
>>
>> The kernel currently maps the the GHES memory region as cacheable
>> (PAGE_KERNEL) for all architectures. This memory region is used as a
>> communication buffer for reporting hardware errors from the firmware to
>> kernel. Essentially the firmware writes hardware error records there,
>> trigger an NMI/interrupt, and the GHES driver goes off and grabs the
>> error record from the GHES region.
>>
>> Since the firmware gets first crack at inspecting the error this
>> mechanism is referred to as "firmware first" in the ACPI spec.
>>
>> Now, there's a mismatch on arm64 platforms between how the kernel maps
>> the GHES region (PAGE_KERNEL) and how the firmware maps it
>> (EFI_MEMORY_UC, i.e. uncacheable), leading to the possibility of the
>> kernel GHES driver reading stale data from the cache when it receives
>> the NMI/interrupt.
>>
>> As for exactly why the arm64 firmware uses an uncached mapping, I
>> presume it's to avoid relying on the kernel to get the necessary cache
>> flushing correct.
>>
>> The proposed solution is to query the EFI memory map to ensure the
>> kernel uses a compatible mapping.
>>
>> None of this should affect x86, it still uses PAGE_KERNEL because we're
>> yet to see any hardware that has an EFI memory map entry for the GHES
>> region that's incompatible with PAGE_KERNEL.
>>
>> Jonathan, would you like to provide more details?
Not really. Matt and Will articulated it to the points.
>
> FWIW, that matches my understanding of the problem too. The ARM architecture
> refers to this situation as "mismatched memory attributes" and typically
> requires some explicit cache maintenance to achieve portable behaviour in
> this scenario.
>
> It's much better to avoid the mismatch in the first place, if you can,
> which is what this is all about.
>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Zhang, Jonathan Zhixiong]

On 8/13/2015 4:14 AM, Will Deacon wrote:
> On Thu, Aug 13, 2015 at 10:24:32AM +0100, Matt Fleming wrote:
>> On Thu, 13 Aug, at 10:19:17AM, Ingo Molnar wrote:
>>> * Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>>>
>>>> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>>>
>>>> With ACPI APEI firmware first handling, generic hardware error
>>>> record is updated by firmware in GHES memory region. On an arm64
>>>> platform, firmware updates GHES memory region with uncached
>>>> access attribute, and then Linux reads stale data from cache.
>>>
>>> So this paragraph does not parse for me ...
I will update the paragraph to explain that with current code,
PAGE_KERNEL is always used, this means that the kernel assumes
cache coherency to the memory region is maintained by firmware;
such assumption is not always correct.
>>>
>>> If it tries to explain a bug it falls very short of doing a proper job of that.
>>
>> I'll let Jonathan provide more details but I understood the problem to
>> be a cache (in)coherency issue.
>>
>> The kernel currently maps the the GHES memory region as cacheable
>> (PAGE_KERNEL) for all architectures. This memory region is used as a
>> communication buffer for reporting hardware errors from the firmware to
>> kernel. Essentially the firmware writes hardware error records there,
>> trigger an NMI/interrupt, and the GHES driver goes off and grabs the
>> error record from the GHES region.
>>
>> Since the firmware gets first crack at inspecting the error this
>> mechanism is referred to as "firmware first" in the ACPI spec.
>>
>> Now, there's a mismatch on arm64 platforms between how the kernel maps
>> the GHES region (PAGE_KERNEL) and how the firmware maps it
>> (EFI_MEMORY_UC, i.e. uncacheable), leading to the possibility of the
>> kernel GHES driver reading stale data from the cache when it receives
>> the NMI/interrupt.
>>
>> As for exactly why the arm64 firmware uses an uncached mapping, I
>> presume it's to avoid relying on the kernel to get the necessary cache
>> flushing correct.
>>
>> The proposed solution is to query the EFI memory map to ensure the
>> kernel uses a compatible mapping.
>>
>> None of this should affect x86, it still uses PAGE_KERNEL because we're
>> yet to see any hardware that has an EFI memory map entry for the GHES
>> region that's incompatible with PAGE_KERNEL.
>>
>> Jonathan, would you like to provide more details?
Not really. Matt and Will articulated it to the points.
>
> FWIW, that matches my understanding of the problem too. The ARM architecture
> refers to this situation as "mismatched memory attributes" and typically
> requires some explicit cache maintenance to achieve portable behaviour in
> this scenario.
>
> It's much better to avoid the mismatch in the first place, if you can,
> which is what this is all about.
>
> Will
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Will Deacon]

On Thu, Aug 13, 2015 at 10:24:32AM +0100, Matt Fleming wrote:
> On Thu, 13 Aug, at 10:19:17AM, Ingo Molnar wrote:
> > * Matt Fleming <matt@codeblueprint.co.uk> 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. On an arm64
> > > platform, firmware updates GHES memory region with uncached
> > > access attribute, and then Linux reads stale data from cache.
> > 
> > So this paragraph does not parse for me ...
> > 
> > If it tries to explain a bug it falls very short of doing a proper job of that.
> 
> I'll let Jonathan provide more details but I understood the problem to
> be a cache (in)coherency issue.
> 
> The kernel currently maps the the GHES memory region as cacheable
> (PAGE_KERNEL) for all architectures. This memory region is used as a
> communication buffer for reporting hardware errors from the firmware to
> kernel. Essentially the firmware writes hardware error records there,
> trigger an NMI/interrupt, and the GHES driver goes off and grabs the
> error record from the GHES region.
> 
> Since the firmware gets first crack at inspecting the error this
> mechanism is referred to as "firmware first" in the ACPI spec.
> 
> Now, there's a mismatch on arm64 platforms between how the kernel maps
> the GHES region (PAGE_KERNEL) and how the firmware maps it
> (EFI_MEMORY_UC, i.e. uncacheable), leading to the possibility of the
> kernel GHES driver reading stale data from the cache when it receives
> the NMI/interrupt.
> 
> As for exactly why the arm64 firmware uses an uncached mapping, I
> presume it's to avoid relying on the kernel to get the necessary cache
> flushing correct.
> 
> The proposed solution is to query the EFI memory map to ensure the
> kernel uses a compatible mapping.
> 
> None of this should affect x86, it still uses PAGE_KERNEL because we're
> yet to see any hardware that has an EFI memory map entry for the GHES
> region that's incompatible with PAGE_KERNEL.
> 
> Jonathan, would you like to provide more details?

FWIW, that matches my understanding of the problem too. The ARM architecture
refers to this situation as "mismatched memory attributes" and typically
requires some explicit cache maintenance to achieve portable behaviour in
this scenario.

It's much better to avoid the mismatch in the first place, if you can,
which is what this is all about.

Will

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Will Deacon]

On Thu, Aug 13, 2015 at 10:24:32AM +0100, Matt Fleming wrote:
> On Thu, 13 Aug, at 10:19:17AM, Ingo Molnar wrote:
> > * Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> > 
> > > From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> > > 
> > > With ACPI APEI firmware first handling, generic hardware error
> > > record is updated by firmware in GHES memory region. On an arm64
> > > platform, firmware updates GHES memory region with uncached
> > > access attribute, and then Linux reads stale data from cache.
> > 
> > So this paragraph does not parse for me ...
> > 
> > If it tries to explain a bug it falls very short of doing a proper job of that.
> 
> I'll let Jonathan provide more details but I understood the problem to
> be a cache (in)coherency issue.
> 
> The kernel currently maps the the GHES memory region as cacheable
> (PAGE_KERNEL) for all architectures. This memory region is used as a
> communication buffer for reporting hardware errors from the firmware to
> kernel. Essentially the firmware writes hardware error records there,
> trigger an NMI/interrupt, and the GHES driver goes off and grabs the
> error record from the GHES region.
> 
> Since the firmware gets first crack at inspecting the error this
> mechanism is referred to as "firmware first" in the ACPI spec.
> 
> Now, there's a mismatch on arm64 platforms between how the kernel maps
> the GHES region (PAGE_KERNEL) and how the firmware maps it
> (EFI_MEMORY_UC, i.e. uncacheable), leading to the possibility of the
> kernel GHES driver reading stale data from the cache when it receives
> the NMI/interrupt.
> 
> As for exactly why the arm64 firmware uses an uncached mapping, I
> presume it's to avoid relying on the kernel to get the necessary cache
> flushing correct.
> 
> The proposed solution is to query the EFI memory map to ensure the
> kernel uses a compatible mapping.
> 
> None of this should affect x86, it still uses PAGE_KERNEL because we're
> yet to see any hardware that has an EFI memory map entry for the GHES
> region that's incompatible with PAGE_KERNEL.
> 
> Jonathan, would you like to provide more details?

FWIW, that matches my understanding of the problem too. The ARM architecture
refers to this situation as "mismatched memory attributes" and typically
requires some explicit cache maintenance to achieve portable behaviour in
this scenario.

It's much better to avoid the mismatch in the first place, if you can,
which is what this is all about.

Will

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Matt Fleming]

On Thu, 13 Aug, at 10:19:17AM, Ingo Molnar wrote:
> 
> * Matt Fleming <matt@codeblueprint.co.uk> 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. On an arm64
> > platform, firmware updates GHES memory region with uncached
> > access attribute, and then Linux reads stale data from cache.
> 
> So this paragraph does not parse for me ...
> 
> If it tries to explain a bug it falls very short of doing a proper job of that.

I'll let Jonathan provide more details but I understood the problem to
be a cache (in)coherency issue.

The kernel currently maps the the GHES memory region as cacheable
(PAGE_KERNEL) for all architectures. This memory region is used as a
communication buffer for reporting hardware errors from the firmware to
kernel. Essentially the firmware writes hardware error records there,
trigger an NMI/interrupt, and the GHES driver goes off and grabs the
error record from the GHES region.

Since the firmware gets first crack at inspecting the error this
mechanism is referred to as "firmware first" in the ACPI spec.

Now, there's a mismatch on arm64 platforms between how the kernel maps
the GHES region (PAGE_KERNEL) and how the firmware maps it
(EFI_MEMORY_UC, i.e. uncacheable), leading to the possibility of the
kernel GHES driver reading stale data from the cache when it receives
the NMI/interrupt.

As for exactly why the arm64 firmware uses an uncached mapping, I
presume it's to avoid relying on the kernel to get the necessary cache
flushing correct.

The proposed solution is to query the EFI memory map to ensure the
kernel uses a compatible mapping.

None of this should affect x86, it still uses PAGE_KERNEL because we're
yet to see any hardware that has an EFI memory map entry for the GHES
region that's incompatible with PAGE_KERNEL.

Jonathan, would you like to provide more details?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Matt Fleming]

On Thu, 13 Aug, at 10:19:17AM, Ingo Molnar wrote:
> 
> * Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> 
> > From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> > 
> > With ACPI APEI firmware first handling, generic hardware error
> > record is updated by firmware in GHES memory region. On an arm64
> > platform, firmware updates GHES memory region with uncached
> > access attribute, and then Linux reads stale data from cache.
> 
> So this paragraph does not parse for me ...
> 
> If it tries to explain a bug it falls very short of doing a proper job of that.

I'll let Jonathan provide more details but I understood the problem to
be a cache (in)coherency issue.

The kernel currently maps the the GHES memory region as cacheable
(PAGE_KERNEL) for all architectures. This memory region is used as a
communication buffer for reporting hardware errors from the firmware to
kernel. Essentially the firmware writes hardware error records there,
trigger an NMI/interrupt, and the GHES driver goes off and grabs the
error record from the GHES region.

Since the firmware gets first crack at inspecting the error this
mechanism is referred to as "firmware first" in the ACPI spec.

Now, there's a mismatch on arm64 platforms between how the kernel maps
the GHES region (PAGE_KERNEL) and how the firmware maps it
(EFI_MEMORY_UC, i.e. uncacheable), leading to the possibility of the
kernel GHES driver reading stale data from the cache when it receives
the NMI/interrupt.

As for exactly why the arm64 firmware uses an uncached mapping, I
presume it's to avoid relying on the kernel to get the necessary cache
flushing correct.

The proposed solution is to query the EFI memory map to ensure the
kernel uses a compatible mapping.

None of this should affect x86, it still uses PAGE_KERNEL because we're
yet to see any hardware that has an EFI memory map entry for the GHES
region that's incompatible with PAGE_KERNEL.

Jonathan, would you like to provide more details?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Ingo Molnar]

* Matt Fleming <matt@codeblueprint.co.uk> 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. On an arm64
> platform, firmware updates GHES memory region with uncached
> access attribute, and then Linux reads stale data from cache.

So this paragraph does not parse for me ...

If it tries to explain a bug it falls very short of doing a proper job of that.

Thanks,

	Ingo

Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Ingo Molnar]

* Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:

> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> 
> With ACPI APEI firmware first handling, generic hardware error
> record is updated by firmware in GHES memory region. On an arm64
> platform, firmware updates GHES memory region with uncached
> access attribute, and then Linux reads stale data from cache.

So this paragraph does not parse for me ...

If it tries to explain a bug it falls very short of doing a proper job of that.

Thanks,

	Ingo

[PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory

[Matt Fleming]

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. On an arm64
platform, firmware updates GHES memory region with uncached
access attribute, and then 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).

Acked-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 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 2bfd53cbfe80..0aa37c57acec 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -164,8 +164,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;
 }
-- 
2.1.0