[PATCH V3 4/4] acpi, apei: use EFI memmap to map GHES memory

[PATCH V3 4/4] acpi, apei: use EFI memmap to map GHES memory

[Jonathan (Zhixiong) Zhang]

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

With ACPI APEI firmware first handling, generic hardware error
record is updated by firmware in GHES memory region. 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 <zjzhang@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;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V3 4/4] acpi, apei: use EFI memmap to map GHES memory

[Borislav Petkov]

On Thu, Jun 11, 2015 at 11:26:00AM -0700, 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. 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 <zjzhang@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;

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.
--

Re: [PATCH V3 4/4] acpi, apei: use EFI memmap to map GHES memory

[Zhang, Jonathan Zhixiong]

On 6/12/2015 9:29 AM, Borislav Petkov wrote:
> On Thu, Jun 11, 2015 at 11:26:00AM -0700, 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. 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 <zjzhang@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;
>
> 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().
 >
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.

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

Re: [PATCH V3 4/4] acpi, apei: use EFI memmap to map GHES memory

[Borislav Petkov]

On Fri, Jun 12, 2015 at 04:44:25PM -0700, Zhang, Jonathan Zhixiong wrote:
> 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.
--

Re: [PATCH V3 4/4] acpi, apei: use EFI memmap to map GHES memory

[Matt Fleming]

On Sat, 13 Jun, at 10:27:51AM, Borislav Petkov wrote:
> On Fri, Jun 12, 2015 at 04:44:25PM -0700, Zhang, Jonathan Zhixiong wrote:
> > 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.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V3 4/4] acpi, apei: use EFI memmap to map GHES memory

[Borislav Petkov]

On Mon, Jun 15, 2015 at 03:15:33PM +0100, Matt Fleming wrote:
> On Sat, 13 Jun, at 10:27:51AM, Borislav Petkov wrote:
> > On Fri, Jun 12, 2015 at 04:44:25PM -0700, Zhang, Jonathan Zhixiong wrote:
> > > 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.
--

Re: [PATCH V3 4/4] acpi, apei: use EFI memmap to map GHES memory

[Matt Fleming]

On Mon, 15 Jun, at 04:59:08PM, Borislav Petkov wrote:
> On Mon, Jun 15, 2015 at 03:15:33PM +0100, Matt Fleming wrote:
> > On Sat, 13 Jun, at 10:27:51AM, Borislav Petkov wrote:
> > > On Fri, Jun 12, 2015 at 04:44:25PM -0700, Zhang, Jonathan Zhixiong wrote:
> > > > 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, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in

Re: [PATCH V3 4/4] acpi, apei: use EFI memmap to map GHES memory

[Matt Fleming]

On Mon, 22 Jun, at 06:11:31AM, Matt Fleming wrote:
> 
> 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?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V3 4/4] acpi, apei: use EFI memmap to map GHES memory

[Zhang, Jonathan Zhixiong]

Hi Matt/Borislav, thanks for the discussion. I am sorry that somehow
I did not see this message in my inbox. I found it by surprise through
an internet search.

> On Mon, 22 Jun, at 06:11:31AM, Matt Fleming wrote:
>>
>> 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?
>
Agree. That being said, I do not know if this GHES case is the only
user case that will benefit from such framework. If it is, then it may
be controversial to introduce a framework for only one use case.

To me, there are two ways that will help GHES case:
a. Define ioremap_page_range_[no]cache() functions for archs, similar
like the case for ioremap_[no]cache.
b. Define a set of PAGE_* protection types (in particular
PAGE_KERNEL_NOCACHE). Right now it seems like only a few protection
types (such as PAGE_KERNEL) are defined across the archs.

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