linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [GIT PULL 0/2] EFI changes for v4.3 (part two)
@ 2015-08-12 16:17 Matt Fleming
  2015-08-12 16:17 ` [PATCH 1/2] arm64: apei: implement arch_apei_get_mem_attributes() Matt Fleming
  2015-08-12 16:17 ` [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory Matt Fleming
  0 siblings, 2 replies; 11+ messages in thread
From: Matt Fleming @ 2015-08-12 16:17 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner, H. Peter Anvin
  Cc: Matt Fleming, linux-kernel, linux-efi, Ard Biesheuvel,
	Borislav Petkov, Catalin Marinas, Hanjun Guo,
	Jonathan (Zhixiong) Zhang

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

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

In this pull is the arm64 arch_apei_get_mem_attributes() implementation
with the changes requested by Ard. The final patch uses the new
arch_apei_get_mem_attributes() in the GHES driver, which means the
driver now maps the GHES region correctly on arm64.

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 tags/efi-next

for you to fetch changes up to 4cf8e8af8ac0a810f05ecc3fce440626b4dcb7e3:

  acpi, apei: use appropriate pgprot_t to map GHES memory (2015-08-12 14:44:46 +0100)

----------------------------------------------------------------
 * Additional patches for arm64 and to the ACPI GHES driver to allow
   architectures to specify which mapping attributes to use to ioremap
   the GHES region - Jonathan (Zhixiong) Zhang

----------------------------------------------------------------
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 | 30 ++++++++++++++++++++++++++++++
 drivers/acpi/apei/ghes.c      |  6 ++++--
 2 files changed, 34 insertions(+), 2 deletions(-)

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

* [PATCH 1/2] arm64: apei: implement arch_apei_get_mem_attributes()
  2015-08-12 16:17 [GIT PULL 0/2] EFI changes for v4.3 (part two) Matt Fleming
@ 2015-08-12 16:17 ` Matt Fleming
  2015-08-13  8:22   ` Ingo Molnar
  2015-08-12 16:17 ` [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory Matt Fleming
  1 sibling, 1 reply; 11+ messages in thread
From: Matt Fleming @ 2015-08-12 16:17 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner, H. Peter Anvin
  Cc: Jonathan (Zhixiong) Zhang, linux-kernel, linux-efi, Matt Fleming,
	Ard Biesheuvel, Catalin Marinas, Hanjun Guo

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>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 arch/arm64/include/asm/acpi.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 406485ed110a..1cbad43e561d 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -19,6 +19,11 @@
 #include <asm/psci.h>
 #include <asm/smp_plat.h>
 
+#ifdef CONFIG_ACPI_APEI
+#include <linux/efi.h>
+#include <asm/pgtable.h>
+#endif
+
 /* Macros for consistency checks of the GICC subtable of MADT */
 #define ACPI_MADT_GICC_LENGTH	\
 	(acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
@@ -92,4 +97,29 @@ static inline const char *acpi_get_enable_method(int cpu)
 {
 	return acpi_psci_present() ? "psci" : NULL;
 }
+
+#ifdef	CONFIG_ACPI_APEI
+static inline 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
+	 * 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
+
 #endif /*_ASM_ACPI_H*/
-- 
2.1.0


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

* [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
  2015-08-12 16:17 [GIT PULL 0/2] EFI changes for v4.3 (part two) Matt Fleming
  2015-08-12 16:17 ` [PATCH 1/2] arm64: apei: implement arch_apei_get_mem_attributes() Matt Fleming
@ 2015-08-12 16:17 ` Matt Fleming
  2015-08-13  8:19   ` Ingo Molnar
  1 sibling, 1 reply; 11+ messages in thread
From: Matt Fleming @ 2015-08-12 16:17 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner, H. Peter Anvin
  Cc: Jonathan (Zhixiong) Zhang, linux-kernel, linux-efi, Matt Fleming,
	Borislav Petkov

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


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

* Re: [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory
  2015-08-12 16:17 ` [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory Matt Fleming
@ 2015-08-13  8:19   ` Ingo Molnar
  2015-08-13  9:24     ` Matt Fleming
  0 siblings, 1 reply; 11+ messages in thread
From: Ingo Molnar @ 2015-08-13  8:19 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Thomas Gleixner, H. Peter Anvin, Jonathan (Zhixiong) Zhang,
	linux-kernel, linux-efi, Matt Fleming, Borislav Petkov


* 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

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

* Re: [PATCH 1/2] arm64: apei: implement arch_apei_get_mem_attributes()
  2015-08-12 16:17 ` [PATCH 1/2] arm64: apei: implement arch_apei_get_mem_attributes() Matt Fleming
@ 2015-08-13  8:22   ` Ingo Molnar
  2015-08-14 18:55     ` Zhang, Jonathan Zhixiong
  0 siblings, 1 reply; 11+ messages in thread
From: Ingo Molnar @ 2015-08-13  8:22 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Thomas Gleixner, H. Peter Anvin, Jonathan (Zhixiong) Zhang,
	linux-kernel, linux-efi, Matt Fleming, Ard Biesheuvel,
	Catalin Marinas, Hanjun Guo


* Matt Fleming <matt@codeblueprint.co.uk> 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>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  arch/arm64/include/asm/acpi.h | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 406485ed110a..1cbad43e561d 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -19,6 +19,11 @@
>  #include <asm/psci.h>
>  #include <asm/smp_plat.h>
>  
> +#ifdef CONFIG_ACPI_APEI
> +#include <linux/efi.h>
> +#include <asm/pgtable.h>
> +#endif
> +
>  /* Macros for consistency checks of the GICC subtable of MADT */
>  #define ACPI_MADT_GICC_LENGTH	\
>  	(acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
> @@ -92,4 +97,29 @@ static inline const char *acpi_get_enable_method(int cpu)
>  {
>  	return acpi_psci_present() ? "psci" : NULL;
>  }
> +
> +#ifdef	CONFIG_ACPI_APEI
> +static inline 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
> +	 * corresponding MAIR attribute encoding.

s/mapped to corresponding/
  mapped to a corresponding

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

Also, this doesn't look like a small function - why is it inlined?

Moving it to .c would also slightly improve compilation times for every .c file 
that includes asm/acpi.h.

Thanks,

	Ingo

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

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

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

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

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

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

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

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



On 8/13/2015 1:22 AM, Ingo Molnar wrote:
>
> * Matt Fleming <matt@codeblueprint.co.uk> 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>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
>> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
>> ---
>>   arch/arm64/include/asm/acpi.h | 30 ++++++++++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
>> index 406485ed110a..1cbad43e561d 100644
>> --- a/arch/arm64/include/asm/acpi.h
>> +++ b/arch/arm64/include/asm/acpi.h
>> @@ -19,6 +19,11 @@
>>   #include <asm/psci.h>
>>   #include <asm/smp_plat.h>
>>
>> +#ifdef CONFIG_ACPI_APEI
>> +#include <linux/efi.h>
>> +#include <asm/pgtable.h>
>> +#endif
>> +
>>   /* Macros for consistency checks of the GICC subtable of MADT */
>>   #define ACPI_MADT_GICC_LENGTH	\
>>   	(acpi_gbl_FADT.header.revision < 6 ? 76 : 80)
>> @@ -92,4 +97,29 @@ static inline const char *acpi_get_enable_method(int cpu)
>>   {
>>   	return acpi_psci_present() ? "psci" : NULL;
>>   }
>> +
>> +#ifdef	CONFIG_ACPI_APEI
>> +static inline 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
>> +	 * corresponding MAIR attribute encoding.
>
> s/mapped to corresponding/
>    mapped to a corresponding
Will do.
>
>> +	 * 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
>
> Also, this doesn't look like a small function - why is it inlined?
The function grew a bit after its first version. Since it is on the
fence of not being a small function, I did not change it to out of
the line. I will move it to arch/arm64/kernel/acpi.c guarded by
CONFIG_ACPI_APEI instead of having to create a very small new file,
if there is no opposition.
>
> Moving it to .c would also slightly improve compilation times for every .c file
> that includes asm/acpi.h.
>
> 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] 11+ messages in thread

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



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

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

* Re: [PATCH 1/2] arm64: apei: implement arch_apei_get_mem_attributes()
  2015-08-14 18:55     ` Zhang, Jonathan Zhixiong
@ 2015-08-14 19:12       ` Matt Fleming
  0 siblings, 0 replies; 11+ messages in thread
From: Matt Fleming @ 2015-08-14 19:12 UTC (permalink / raw)
  To: Zhang, Jonathan Zhixiong
  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 11:55:27AM, Zhang, Jonathan Zhixiong wrote:
> 
> >Also, this doesn't look like a small function - why is it inlined?
> The function grew a bit after its first version. Since it is on the
> fence of not being a small function, I did not change it to out of
> the line. I will move it to arch/arm64/kernel/acpi.c guarded by
> CONFIG_ACPI_APEI instead of having to create a very small new file,
> if there is no opposition.

Please keep Will in the loop (now Cc'd) since he originally suggested
moving this to a header file and marking it static inline.

-- 
Matt Fleming, Intel Open Source Technology Center

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

* [PATCH 1/2] arm64: apei: Implement arch_apei_get_mem_attributes()
  2015-09-04 13:11 [GIT PULL 0/2] EFI changes for v4.3 (part two) Matt Fleming
@ 2015-09-04 13:11 ` Matt Fleming
  0 siblings, 0 replies; 11+ messages in thread
From: Matt Fleming @ 2015-09-04 13:11 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner, H. Peter Anvin
  Cc: Jonathan (Zhixiong) Zhang, linux-kernel, linux-efi, Will Deacon,
	Matt Fleming, Ard Biesheuvel, Catalin Marinas, Hanjun Guo

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


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

end of thread, other threads:[~2015-09-04 13:11 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-12 16:17 [GIT PULL 0/2] EFI changes for v4.3 (part two) Matt Fleming
2015-08-12 16:17 ` [PATCH 1/2] arm64: apei: implement arch_apei_get_mem_attributes() Matt Fleming
2015-08-13  8:22   ` Ingo Molnar
2015-08-14 18:55     ` Zhang, Jonathan Zhixiong
2015-08-14 19:12       ` Matt Fleming
2015-08-12 16:17 ` [PATCH 2/2] acpi, apei: use appropriate pgprot_t to map GHES memory Matt Fleming
2015-08-13  8:19   ` Ingo Molnar
2015-08-13  9:24     ` Matt Fleming
2015-08-13 11:14       ` Will Deacon
2015-08-14 19:09         ` Zhang, Jonathan Zhixiong
2015-09-04 13:11 [GIT PULL 0/2] EFI changes for v4.3 (part two) Matt Fleming
2015-09-04 13:11 ` [PATCH 1/2] arm64: apei: Implement arch_apei_get_mem_attributes() Matt Fleming

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).