From: Matt Fleming <[email protected]>
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(-)
From: "Jonathan (Zhixiong) Zhang" <[email protected]>
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 <[email protected]>
Acked-by: Hanjun Guo <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Jonathan (Zhixiong) Zhang <[email protected]>
Signed-off-by: Matt Fleming <[email protected]>
---
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
From: "Jonathan (Zhixiong) Zhang" <[email protected]>
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 <[email protected]>
Signed-off-by: Jonathan (Zhixiong) Zhang <[email protected]>
Signed-off-by: Matt Fleming <[email protected]>
---
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
* Matt Fleming <[email protected]> wrote:
> From: "Jonathan (Zhixiong) Zhang" <[email protected]>
>
> 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
* Matt Fleming <[email protected]> wrote:
> From: "Jonathan (Zhixiong) Zhang" <[email protected]>
>
> 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 <[email protected]>
> Acked-by: Hanjun Guo <[email protected]>
> Reviewed-by: Ard Biesheuvel <[email protected]>
> Signed-off-by: Jonathan (Zhixiong) Zhang <[email protected]>
> Signed-off-by: Matt Fleming <[email protected]>
> ---
> 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
On Thu, 13 Aug, at 10:19:17AM, Ingo Molnar wrote:
>
> * Matt Fleming <[email protected]> wrote:
>
> > From: "Jonathan (Zhixiong) Zhang" <[email protected]>
> >
> > 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
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 <[email protected]> wrote:
> >
> > > From: "Jonathan (Zhixiong) Zhang" <[email protected]>
> > >
> > > 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
On 8/13/2015 1:22 AM, Ingo Molnar wrote:
>
> * Matt Fleming <[email protected]> wrote:
>
>> From: "Jonathan (Zhixiong) Zhang" <[email protected]>
>>
>> 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 <[email protected]>
>> Acked-by: Hanjun Guo <[email protected]>
>> Reviewed-by: Ard Biesheuvel <[email protected]>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <[email protected]>
>> Signed-off-by: Matt Fleming <[email protected]>
>> ---
>> 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
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 <[email protected]> wrote:
>>>
>>>> From: "Jonathan (Zhixiong) Zhang" <[email protected]>
>>>>
>>>> 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 [email protected]
> 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
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