[PATCH V3 0/4] map GHES memory region with EFI memory map

[Jonathan (Zhixiong) Zhang]

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

On a platform with APEI (ACPI Platform Error Interface) enabled, firmware
updates a memory region with hardware error record using nocache
attribute. When OS reads the region, since it maps the region with
cacahed attribute even though EFI memory map defines this region as
uncached, OS gets stale data and errorneously reports there is no new
HW error.

When ghes driver maps the memory region, it uses the cache attribute
according to EFI memory map, if EFI memory map feature is enabled
at runtime.

Since both arch/x86 and arch/ia64 implemented architecture agnostic EFI
memory map attribue lookup function efi_memattributes(), the code is
moved from arch/x86 into EFI subsystem and is declared as __weak; archs
other than ia64 should not override the default implementation.

V3:
1. Rebased to v4.1-rc7.
2. Moved efi_mem_attributes() from arch/x86 to drivers/firmware/efi
and declared it as __weak.
3. Introduced ARCH_APEI_PAGE_KERNEL_UC to allow arch specific page
protection type for UC.
4. Removed efi_ioremap(). It can not be used for GHES memory region
mapping purpose since ioremap can not be used in atomic context.

V2:
1. Rebased to v4.1-rc5.
2. Split removal of efi_mem_attributes() and creation of efi_ioremap()
into two patches.

Jonathan (Zhixiong) Zhang (4):
x86: acpi: define uncached page flag
arm64: acpi: define uncached page flag
efi: x86: rearrange efi_mem_attributes()
acpi, apei: use EFI memmap to map GHES memory

arch/arm64/include/asm/acpi.h | 4 ++++
arch/x86/include/asm/acpi.h | 4 ++++
arch/x86/platform/efi/efi.c | 18 ------------------
drivers/acpi/apei/ghes.c | 9 ++++++++-
drivers/firmware/efi/efi.c | 18 ++++++++++++++++++
5 files changed, 34 insertions(+), 19 deletions(-)

[Jonathan (Zhixiong) Zhang]

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

This defintion is used in ACPI code when needing to map page
as uncached.

Signed-off-by: Jonathan (Zhixiong) Zhang <***@codeaurora.org>
---
arch/arm64/include/asm/acpi.h | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 59c05d8ea4a0..518b91dc5acd 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -93,4 +93,8 @@ static inline bool acpi_psci_use_hvc(void) { return false; }
static inline void acpi_init_cpus(void) { }
#endif /* CONFIG_ACPI */

+#ifdef CONFIG_ACPI_APEI
+#define ARCH_APEI_PAGE_KERNEL_UC (PROT_DEVICE_nGnRE)
+#endif /*CONFIG_ACPI_APEI */
+
#endif /*_ASM_ACPI_H*/

[Jonathan (Zhixiong) Zhang]

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

With ACPI APEI firmware first handling, generic hardware error
record is updated by firmware in GHES memory region. When firmware
updated GHES memory region with uncached access attribute, Linux
reads stale data from cache.

GHES memory region should be mapped with cache attributes
according to EFI memory map when applicable. On such system, if
firmware updates RAM directly without going through cache,
EFI memory map has GHES memory region defined as uncached;
If firmware updates cache, EFI memory map has GHES memory region
defined as cached.

On EFI system, if GHES memory region has EFI_MEMORY_UC attribute
defined, map the page with arch defined UC page protection type.

Signed-off-by: Jonathan (Zhixiong) Zhang <***@codeaurora.org>
---
drivers/acpi/apei/ghes.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index e82d0976a5d0..5cfd951ebf67 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -48,6 +48,7 @@
#include <linux/pci.h>
#include <linux/aer.h>
#include <linux/nmi.h>
+#include <linux/efi.h>

#include <acpi/ghes.h>
#include <acpi/apei.h>
@@ -164,8 +165,14 @@ 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,
+
+ if (efi_mem_attributes(pfn << PAGE_SHIFT) & EFI_MEMORY_UC) {
+ ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
+ pfn << PAGE_SHIFT, ARCH_APEI_PAGE_KERNEL_UC);
+ } else {
+ ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
pfn << PAGE_SHIFT, PAGE_KERNEL);
+ }

return (void __iomem *)vaddr;
}

[Borislav Petkov]

Post by Jonathan (Zhixiong) Zhang
With ACPI APEI firmware first handling, generic hardware error
record is updated by firmware in GHES memory region. When firmware
updated GHES memory region with uncached access attribute, Linux
reads stale data from cache.
GHES memory region should be mapped with cache attributes
according to EFI memory map when applicable. On such system, if
firmware updates RAM directly without going through cache,
EFI memory map has GHES memory region defined as uncached;
If firmware updates cache, EFI memory map has GHES memory region
defined as cached.
On EFI system, if GHES memory region has EFI_MEMORY_UC attribute
defined, map the page with arch defined UC page protection type.
---
drivers/acpi/apei/ghes.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index e82d0976a5d0..5cfd951ebf67 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -48,6 +48,7 @@
#include <linux/pci.h>
#include <linux/aer.h>
#include <linux/nmi.h>
+#include <linux/efi.h>
#include <acpi/ghes.h>
#include <acpi/apei.h>
@@ -164,8 +165,14 @@ 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,
+
+ if (efi_mem_attributes(pfn << PAGE_SHIFT) & EFI_MEMORY_UC) {
+ ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
+ pfn << PAGE_SHIFT, ARCH_APEI_PAGE_KERNEL_UC);
+ } else {
+ ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
pfn << PAGE_SHIFT, PAGE_KERNEL);
+ }
return (void __iomem *)vaddr;

This is still hacky. All of a sudden, ghes code gets to know about EFI
which looks like a mishmash to me.

Maybe it would be cleaner if you added an arch-specific

arch_get_mem_attribute(phys_addr);

which returns a pgprot_t thing which you can use directly above, like
so:

ioremap_page_range(vaddr,
vaddr + PAGE_SIZE,
pfn << PAGE_SHIFT,
arch_get_mem_attribute(pfn << PAGE_SHIFT));

Each arch can then return what it likes with arch_get_mem_attribute().
--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Zhang, Jonathan Zhixiong]

Post by Borislav Petkov

Post by Jonathan (Zhixiong) Zhang
With ACPI APEI firmware first handling, generic hardware error
record is updated by firmware in GHES memory region. When firmware
updated GHES memory region with uncached access attribute, Linux
reads stale data from cache.
GHES memory region should be mapped with cache attributes
according to EFI memory map when applicable. On such system, if
firmware updates RAM directly without going through cache,
EFI memory map has GHES memory region defined as uncached;
If firmware updates cache, EFI memory map has GHES memory region
defined as cached.
On EFI system, if GHES memory region has EFI_MEMORY_UC attribute
defined, map the page with arch defined UC page protection type.
---
drivers/acpi/apei/ghes.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index e82d0976a5d0..5cfd951ebf67 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -48,6 +48,7 @@
#include <linux/pci.h>
#include <linux/aer.h>
#include <linux/nmi.h>
+#include <linux/efi.h>
#include <acpi/ghes.h>
#include <acpi/apei.h>
@@ -164,8 +165,14 @@ 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,
+
+ if (efi_mem_attributes(pfn << PAGE_SHIFT) & EFI_MEMORY_UC) {
+ ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
+ pfn << PAGE_SHIFT, ARCH_APEI_PAGE_KERNEL_UC);
+ } else {
+ ioremap_page_range(vaddr, vaddr + PAGE_SIZE,
pfn << PAGE_SHIFT, PAGE_KERNEL);
+ }
return (void __iomem *)vaddr;

This is still hacky. All of a sudden, ghes code gets to know about EFI
which looks like a mishmash to me.
Maybe it would be cleaner if you added an arch-specific
arch_get_mem_attribute(phys_addr);
which returns a pgprot_t thing which you can use directly above, like
ioremap_page_range(vaddr,
vaddr + PAGE_SIZE,
pfn << PAGE_SHIFT,
arch_get_mem_attribute(pfn << PAGE_SHIFT));
Each arch can then return what it likes with arch_get_mem_attribute().

Thanks for the review, Borislav. I will do that. Since such function
is only needed for APEI functionality, at least as of today, I will
name it arch_apei_get_mem_attribute().
I will add implementations for arm64 and x86 since those are the only
archs having HAVE_ACPI_APEI defined in Kconfig. The upstream kernel
today does not have HAVE_ACPI_APEI defined for arm64, but it needs to
be and will be.

[Borislav Petkov]

Since such function is only needed for APEI functionality, at least as
of today, I will name it arch_apei_get_mem_attribute().

Why?

It can be extended to be used generically too, no? Come to think of it,
the different arches should already have a way to tell you with what mem
attributes a physical address is mapped, no?

IOW, such functionality should be already present, you'd only have to
find it and use it.
--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Matt Fleming]

Post by Borislav Petkov

Since such function is only needed for APEI functionality, at least as
of today, I will name it arch_apei_get_mem_attribute().

Why?
It can be extended to be used generically too, no? Come to think of it,
the different arches should already have a way to tell you with what mem
attributes a physical address is mapped, no?
IOW, such functionality should be already present, you'd only have to
find it and use it.

I did think about this, but I don't think we have a generic way to ask
the firmware for its memory map.

The problem on x86 of course is that we simply ignore what the firmware
tells us most of the time because everything gets funneled through the
e820 map and we eventually discard the EFI memmap.

[Borislav Petkov]

Post by Matt Fleming

Post by Borislav Petkov

Since such function is only needed for APEI functionality, at least as
of today, I will name it arch_apei_get_mem_attribute().

Why?
It can be extended to be used generically too, no? Come to think of it,
the different arches should already have a way to tell you with what mem
attributes a physical address is mapped, no?
IOW, such functionality should be already present, you'd only have to
find it and use it.

I did think about this, but I don't think we have a generic way to ask
the firmware for its memory map.

Not the firmware but the OS. Like on x86, for example, we have MTRRs
and PAT and they cover the whole range. Basically what lookup_memtype()
does. We already have that info, why not query it instead of growing
more stuff ontop.

I mean, we do ioremap* which does reserve_memtype() and sticks the range
in the rbtree which we query after. Can't be better than that...

Hmmm.
--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Matt Fleming]

Post by Borislav Petkov

Post by Matt Fleming

Post by Borislav Petkov

Since such function is only needed for APEI functionality, at least as
of today, I will name it arch_apei_get_mem_attribute().

Why?
It can be extended to be used generically too, no? Come to think of it,
the different arches should already have a way to tell you with what mem
attributes a physical address is mapped, no?
IOW, such functionality should be already present, you'd only have to
find it and use it.

I did think about this, but I don't think we have a generic way to ask
the firmware for its memory map.

Not the firmware but the OS. Like on x86, for example, we have MTRRs
and PAT and they cover the whole range. Basically what lookup_memtype()
does. We already have that info, why not query it instead of growing
more stuff ontop.
I mean, we do ioremap* which does reserve_memtype() and sticks the range
in the rbtree which we query after. Can't be better than that...

Right, but see my previous comment about x86 discarding a bunch of
attributes for memory regions because the kernel "knows better".

And in most places, yes, the kernel really does know better. But this
APEI case is special because irrespective of what the kernel says we
want to be compatible with the firmware's memory map.

And we don't have an API for that.

[Matt Fleming]

Post by Matt Fleming
Right, but see my previous comment about x86 discarding a bunch of
attributes for memory regions because the kernel "knows better".
And in most places, yes, the kernel really does know better. But this
APEI case is special because irrespective of what the kernel says we
want to be compatible with the firmware's memory map.
And we don't have an API for that.

Maybe what we want is a new PAGE_* protection that is compatible with
any firmware mappings? That'd be nice because we wouldn't have to
introduce a whole new API for this GHES case and ioremap_* could do
whatever it wanted under the hood.

Thougts?

[Jonathan (Zhixiong) Zhang]

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

x86 and ia64 implement efi_mem_attributes() differently. This
function needs to be available for other arch (such as arm64)
as well, such as for the purpose of ACPI/APEI.

ia64 efi does not setup memmap variable and does not set
EFI_MEMMAP flag, so it needs to have its unique implementation
of efi_mem_attributes().

Move efi_mem_attributes() implementation from x86 to efi, and
declare it with __weak. It is recommended that other archs
should not override the default implementation.

Signed-off-by: Jonathan (Zhixiong) Zhang <***@codeaurora.org>
---
arch/x86/platform/efi/efi.c | 18 ------------------
drivers/firmware/efi/efi.c | 18 ++++++++++++++++++
2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 02744df576d5..10bd5289c593 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -926,24 +926,6 @@ u32 efi_mem_type(unsigned long phys_addr)
return 0;
}

-u64 efi_mem_attributes(unsigned long phys_addr)
-{
- efi_memory_desc_t *md;
- void *p;
-
- if (!efi_enabled(EFI_MEMMAP))
- return 0;
-
- for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
- md = p;
- if ((md->phys_addr <= phys_addr) &&
- (phys_addr < (md->phys_addr +
- (md->num_pages << EFI_PAGE_SHIFT))))
- return md->attribute;
- }
- return 0;
-}
-
static int __init arch_parse_efi_cmdline(char *str)
{
if (parse_option_str(str, "old_map"))
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3061bb8629dc..88531b0dee80 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -517,3 +517,21 @@ char * __init efi_md_typeattr_format(char *buf, size_t size,
attr & EFI_MEMORY_UC ? "UC" : "");
return buf;
}
+
+u64 __weak efi_mem_attributes(unsigned long phys_addr)
+{
+ efi_memory_desc_t *md;
+ void *p;
+
+ if (!efi_enabled(EFI_MEMMAP))
+ return 0;
+
+ for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
+ md = p;
+ if ((md->phys_addr <= phys_addr) &&
+ (phys_addr < (md->phys_addr +
+ (md->num_pages << EFI_PAGE_SHIFT))))
+ return md->attribute;
+ }
+ return 0;
+}

[Jonathan (Zhixiong) Zhang]

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

This definition is used in APEI code when needing to map pages as
uncached.

Signed-off-by: Jonathan (Zhixiong) Zhang <***@codeaurora.org>
---
arch/x86/include/asm/acpi.h | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index 3a45668f6dc3..6afb27682b00 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -138,6 +138,10 @@ static inline void disable_acpi(void) { }

#endif /* !CONFIG_ACPI */

+#ifdef CONFIG_ACPI_APEI
+#define ARCH_APEI_PAGE_KERNEL_UC (PAGE_KERNEL_NOCACHE)
+#endif /*CONFIG_ACPI_APEI */
+
#define ARCH_HAS_POWER_INIT 1

#ifdef CONFIG_ACPI_NUMA

[Ingo Molnar]

Post by Jonathan (Zhixiong) Zhang
This definition is used in APEI code when needing to map pages as
uncached.
---
arch/x86/include/asm/acpi.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index 3a45668f6dc3..6afb27682b00 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -138,6 +138,10 @@ static inline void disable_acpi(void) { }
#endif /* !CONFIG_ACPI */
+#ifdef CONFIG_ACPI_APEI
+#define ARCH_APEI_PAGE_KERNEL_UC (PAGE_KERNEL_NOCACHE)
+#endif /*CONFIG_ACPI_APEI */

- Why the tab?

- Why the parentheses?

- Why the closing comment block for a oneliner define? It's not like we forget
what it's about.

- When the missing space in the closing comment?

That's 4 uglies in 3 lines of code, wow ...

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Zhang, Jonathan Zhixiong]

Post by Ingo Molnar

Post by Jonathan (Zhixiong) Zhang
This definition is used in APEI code when needing to map pages as
uncached.
---
arch/x86/include/asm/acpi.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index 3a45668f6dc3..6afb27682b00 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -138,6 +138,10 @@ static inline void disable_acpi(void) { }
#endif /* !CONFIG_ACPI */
+#ifdef CONFIG_ACPI_APEI
+#define ARCH_APEI_PAGE_KERNEL_UC (PAGE_KERNEL_NOCACHE)
+#endif /*CONFIG_ACPI_APEI */

- Why the tab?
- Why the parentheses?
- Why the closing comment block for a oneliner define? It's not like we forget
what it's about.
- When the missing space in the closing comment?
That's 4 uglies in 3 lines of code, wow ...
Thanks,
Ingo

Thank you Ingo for the review. I agree that style is important.