All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 1/2] arm64: apei: implement arch_apei_get_mem_attributes()
@ 2015-08-14 22:37 ` Jonathan (Zhixiong) Zhang
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan (Zhixiong) Zhang @ 2015-08-14 22:37 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner, H . Peter Anvin, linux-kernel,
	linux-efi, Matt Fleming, Ard Biesheuvel, Catalin Marinas,
	Hanjun Guo, Will Deacon
  Cc: Jonathan (Zhixiong) Zhang

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: Catalin Marinas <catalin.marinas@arm.com>
Acked-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Matt Fleming <matt.fleming@intel.com>
Cc: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Will Deacon <will.deacon@arm.com>
---
V2: Changed arm64's implementation of arch_apei_get_mem_attributes()
    from inline function to out of line function, based on Ingo's
    feedback.

 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..8084f3640006 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
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH V2 1/2] arm64: apei: implement arch_apei_get_mem_attributes()
@ 2015-08-14 22:37 ` Jonathan (Zhixiong) Zhang
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan (Zhixiong) Zhang @ 2015-08-14 22:37 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner, H . Peter Anvin,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-efi-u79uwXL29TY76Z2rM5mHXA, Matt Fleming, Ard Biesheuvel,
	Catalin Marinas, Hanjun Guo, Will Deacon
  Cc: Jonathan (Zhixiong) Zhang

From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
Acked-by: Hanjun Guo <hanjun.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Hanjun Guo <hanjun.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
V2: Changed arm64's implementation of arch_apei_get_mem_attributes()
    from inline function to out of line function, based on Ingo's
    feedback.

 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..8084f3640006 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
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
  2015-08-14 22:37 ` Jonathan (Zhixiong) Zhang
  (?)
@ 2015-08-14 22:37 ` Jonathan (Zhixiong) Zhang
  2015-08-17 13:13     ` Matt Fleming
  2015-08-22  9:24     ` Ingo Molnar
  -1 siblings, 2 replies; 19+ messages in thread
From: Jonathan (Zhixiong) Zhang @ 2015-08-14 22:37 UTC (permalink / raw)
  To: Will Deacon, Ingo Molnar, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas, Matt Fleming
  Cc: 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH V2 1/2] arm64: apei: implement arch_apei_get_mem_attributes()
@ 2015-08-17 13:05   ` Matt Fleming
  0 siblings, 0 replies; 19+ messages in thread
From: Matt Fleming @ 2015-08-17 13:05 UTC (permalink / raw)
  To: Jonathan (Zhixiong) Zhang
  Cc: Ingo Molnar, Thomas Gleixner, H . Peter Anvin, linux-kernel,
	linux-efi, Matt Fleming, Ard Biesheuvel, Catalin Marinas,
	Hanjun Guo, Will Deacon

On Fri, 14 Aug, at 03:37:29PM, Jonathan (Zhixiong) Zhang wrote:
> 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: Catalin Marinas <catalin.marinas@arm.com>
> Acked-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Matt Fleming <matt.fleming@intel.com>
> Cc: Hanjun Guo <hanjun.guo@linaro.org>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
> V2: Changed arm64's implementation of arch_apei_get_mem_attributes()
>     from inline function to out of line function, based on Ingo's
>     feedback.
> 
>  arch/arm64/include/asm/acpi.h |  5 +++++
>  arch/arm64/kernel/acpi.c      | 29 +++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
> 

Looks fine to me, though it appears to be missing Ard's Reviewed-by
tag (the only change from the previous version is that it's no longer
inline).

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

Ingo, are you going to pick up this patch series and apply it directly
to tip if you're OK with this version?

-- 
Matt Fleming, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V2 1/2] arm64: apei: implement arch_apei_get_mem_attributes()
@ 2015-08-17 13:05   ` Matt Fleming
  0 siblings, 0 replies; 19+ messages in thread
From: Matt Fleming @ 2015-08-17 13:05 UTC (permalink / raw)
  To: Jonathan (Zhixiong) Zhang
  Cc: Ingo Molnar, Thomas Gleixner, H . Peter Anvin,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-efi-u79uwXL29TY76Z2rM5mHXA, Matt Fleming, Ard Biesheuvel,
	Catalin Marinas, Hanjun Guo, Will Deacon

On Fri, 14 Aug, at 03:37:29PM, Jonathan (Zhixiong) Zhang wrote:
> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> Acked-by: Hanjun Guo <hanjun.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Cc: Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Hanjun Guo <hanjun.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> ---
> V2: Changed arm64's implementation of arch_apei_get_mem_attributes()
>     from inline function to out of line function, based on Ingo's
>     feedback.
> 
>  arch/arm64/include/asm/acpi.h |  5 +++++
>  arch/arm64/kernel/acpi.c      | 29 +++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
> 

Looks fine to me, though it appears to be missing Ard's Reviewed-by
tag (the only change from the previous version is that it's no longer
inline).

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

Ingo, are you going to pick up this patch series and apply it directly
to tip if you're OK with this version?

-- 
Matt Fleming, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
@ 2015-08-17 13:13     ` Matt Fleming
  0 siblings, 0 replies; 19+ messages in thread
From: Matt Fleming @ 2015-08-17 13:13 UTC (permalink / raw)
  To: Jonathan (Zhixiong) Zhang
  Cc: Will Deacon, Ingo Molnar, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
@ 2015-08-17 13:13     ` Matt Fleming
  0 siblings, 0 replies; 19+ messages in thread
From: Matt Fleming @ 2015-08-17 13:13 UTC (permalink / raw)
  To: Jonathan (Zhixiong) Zhang
  Cc: Will Deacon, Ingo Molnar, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V2 1/2] arm64: apei: implement arch_apei_get_mem_attributes()
@ 2015-08-17 21:09     ` Zhang, Jonathan Zhixiong
  0 siblings, 0 replies; 19+ messages in thread
From: Zhang, Jonathan Zhixiong @ 2015-08-17 21:09 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Ingo Molnar, Thomas Gleixner, H . Peter Anvin, linux-kernel,
	linux-efi, Matt Fleming, Ard Biesheuvel, Catalin Marinas,
	Hanjun Guo, Will Deacon



On 8/17/2015 6:05 AM, Matt Fleming wrote:
> On Fri, 14 Aug, at 03:37:29PM, Jonathan (Zhixiong) Zhang wrote:
>> 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: Catalin Marinas <catalin.marinas@arm.com>
>> Acked-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Matt Fleming <matt.fleming@intel.com>
>> Cc: Hanjun Guo <hanjun.guo@linaro.org>
>> Cc: Will Deacon <will.deacon@arm.com>
>> ---
>> V2: Changed arm64's implementation of arch_apei_get_mem_attributes()
>>      from inline function to out of line function, based on Ingo's
>>      feedback.
>>
>>   arch/arm64/include/asm/acpi.h |  5 +++++
>>   arch/arm64/kernel/acpi.c      | 29 +++++++++++++++++++++++++++++
>>   2 files changed, 34 insertions(+)
>>
>
> Looks fine to me, though it appears to be missing Ard's Reviewed-by
> tag (the only change from the previous version is that it's no longer
> inline).
>
> Reviewed-by: Matt Fleming <matt.fleming@intel.com>
Thanks. Will add both Reviewed-by.
>
> Ingo, are you going to pick up this patch series and apply it directly
> to tip if you're OK with this version?
>

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH V2 1/2] arm64: apei: implement arch_apei_get_mem_attributes()
@ 2015-08-17 21:09     ` Zhang, Jonathan Zhixiong
  0 siblings, 0 replies; 19+ messages in thread
From: Zhang, Jonathan Zhixiong @ 2015-08-17 21:09 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Ingo Molnar, Thomas Gleixner, H . Peter Anvin,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-efi-u79uwXL29TY76Z2rM5mHXA, Matt Fleming, Ard Biesheuvel,
	Catalin Marinas, Hanjun Guo, Will Deacon



On 8/17/2015 6:05 AM, Matt Fleming wrote:
> On Fri, 14 Aug, at 03:37:29PM, Jonathan (Zhixiong) Zhang wrote:
>> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
>> Acked-by: Hanjun Guo <hanjun.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> Cc: Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Cc: Hanjun Guo <hanjun.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
>> ---
>> V2: Changed arm64's implementation of arch_apei_get_mem_attributes()
>>      from inline function to out of line function, based on Ingo's
>>      feedback.
>>
>>   arch/arm64/include/asm/acpi.h |  5 +++++
>>   arch/arm64/kernel/acpi.c      | 29 +++++++++++++++++++++++++++++
>>   2 files changed, 34 insertions(+)
>>
>
> Looks fine to me, though it appears to be missing Ard's Reviewed-by
> tag (the only change from the previous version is that it's no longer
> inline).
>
> Reviewed-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Thanks. Will add both Reviewed-by.
>
> Ingo, are you going to pick up this patch series and apply it directly
> to tip if you're OK with this version?
>

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
@ 2015-08-17 21:10       ` Zhang, Jonathan Zhixiong
  0 siblings, 0 replies; 19+ messages in thread
From: Zhang, Jonathan Zhixiong @ 2015-08-17 21:10 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Will Deacon, Ingo Molnar, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
@ 2015-08-17 21:10       ` Zhang, Jonathan Zhixiong
  0 siblings, 0 replies; 19+ messages in thread
From: Zhang, Jonathan Zhixiong @ 2015-08-17 21:10 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Will Deacon, Ingo Molnar, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
@ 2015-08-22  9:24     ` Ingo Molnar
  0 siblings, 0 replies; 19+ messages in thread
From: Ingo Molnar @ 2015-08-22  9:24 UTC (permalink / raw)
  To: Jonathan (Zhixiong) Zhang
  Cc: Will Deacon, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas, Matt Fleming


* 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
@ 2015-08-22  9:24     ` Ingo Molnar
  0 siblings, 0 replies; 19+ messages in thread
From: Ingo Molnar @ 2015-08-22  9:24 UTC (permalink / raw)
  To: Jonathan (Zhixiong) Zhang
  Cc: Will Deacon, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas, Matt Fleming


* 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
@ 2015-08-24 18:22       ` Zhang, Jonathan Zhixiong
  0 siblings, 0 replies; 19+ messages in thread
From: Zhang, Jonathan Zhixiong @ 2015-08-24 18:22 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Will Deacon, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas, Matt Fleming



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
@ 2015-08-24 18:22       ` Zhang, Jonathan Zhixiong
  0 siblings, 0 replies; 19+ messages in thread
From: Zhang, Jonathan Zhixiong @ 2015-08-24 18:22 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Will Deacon, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas, Matt Fleming



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
@ 2015-08-25  8:59         ` Ingo Molnar
  0 siblings, 0 replies; 19+ messages in thread
From: Ingo Molnar @ 2015-08-25  8:59 UTC (permalink / raw)
  To: Zhang, Jonathan Zhixiong
  Cc: Will Deacon, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas, Matt Fleming


* 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
@ 2015-08-25  8:59         ` Ingo Molnar
  0 siblings, 0 replies; 19+ messages in thread
From: Ingo Molnar @ 2015-08-25  8:59 UTC (permalink / raw)
  To: Zhang, Jonathan Zhixiong
  Cc: Will Deacon, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas, Matt Fleming


* 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
@ 2015-08-25 17:30           ` Zhang, Jonathan Zhixiong
  0 siblings, 0 replies; 19+ messages in thread
From: Zhang, Jonathan Zhixiong @ 2015-08-25 17:30 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Will Deacon, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas, Matt Fleming



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
@ 2015-08-25 17:30           ` Zhang, Jonathan Zhixiong
  0 siblings, 0 replies; 19+ messages in thread
From: Zhang, Jonathan Zhixiong @ 2015-08-25 17:30 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Will Deacon, Thomas Gleixner, H . Peter Anvin,
	linux-kernel @ vger . kernel . org,
	linux-efi @ vger . kernel . org, Matt Fleming, Borislav Petkov,
	Ard Biesheuvel, Catalin Marinas, Matt Fleming



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2015-08-25 17:30 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-14 22:37 [PATCH V2 1/2] arm64: apei: implement arch_apei_get_mem_attributes() Jonathan (Zhixiong) Zhang
2015-08-14 22:37 ` Jonathan (Zhixiong) Zhang
2015-08-14 22:37 ` [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory Jonathan (Zhixiong) Zhang
2015-08-17 13:13   ` Matt Fleming
2015-08-17 13:13     ` Matt Fleming
2015-08-17 21:10     ` Zhang, Jonathan Zhixiong
2015-08-17 21:10       ` Zhang, Jonathan Zhixiong
2015-08-22  9:24   ` Ingo Molnar
2015-08-22  9:24     ` Ingo Molnar
2015-08-24 18:22     ` Zhang, Jonathan Zhixiong
2015-08-24 18:22       ` Zhang, Jonathan Zhixiong
2015-08-25  8:59       ` Ingo Molnar
2015-08-25  8:59         ` Ingo Molnar
2015-08-25 17:30         ` Zhang, Jonathan Zhixiong
2015-08-25 17:30           ` Zhang, Jonathan Zhixiong
2015-08-17 13:05 ` [PATCH V2 1/2] arm64: apei: implement arch_apei_get_mem_attributes() Matt Fleming
2015-08-17 13:05   ` Matt Fleming
2015-08-17 21:09   ` Zhang, Jonathan Zhixiong
2015-08-17 21:09     ` Zhang, Jonathan Zhixiong

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.