[PATCH V9 0/5] map GHES memory region according to EFI memory map

[PATCH V9 0/5] map GHES memory region according to EFI memory map

[Jonathan (Zhixiong) Zhang]

From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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.

V9:
1. Rebased to arm64-upstream-14543 of arm64/master.
2. Match strict MM type in arch_apei_get_mem_attribute().
V8:
1. For x86, always return PAGE_KERNEL for arch_apei_get_mem_attribute().
   The rational is explained in comment.
2. Rebased to arm64-upstream-14201 of arm64/master,
   next-20150724 of linux-next/master.
V7:
1. Added PROT_DEVICE_nGnRnE and PROT_NORMAL_WT to support all
   possible UEFI memory types for arm64.
V6:
1. Implemented arch_apei_get_mem_attributes() for arm64 as inline
   function.
2. Rebased to efi-next-14364 of efi/next, pm+acpi-4.2-rc3 of
   linux-pm/master, arm64-upstream-13521 of arm64/master,
   next-20150720 of linux-next/master.
V5:
1. Rebased to next-20150713 of linux-next/master, efi-next-14359 of
   efi/next, pm+acpi-4.2-rc2 of linux-pm/master,
   arm64-fixes-1215 of arm64/master.
2. Added comment for efi_mem_attributes(), explained why it is marked
   as __weak at the function definition site.
V4:
1. Introduced arch_apei_get_mem_attributes() to allow arch specific
   implementation of getting pgprot_t appropriate for a physical
   address.
2. Implemented arch_apei_get_mem_attributes() for x86 and for arm64.
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 (5):
  efi: x86: rearrange efi_mem_attributes()
  x86: acpi: implement arch_apei_get_mem_attributes()
  arm64: mm: add PROT_DEVICE_nGnRnE and PROT_NORMAL_WT
  arm64: apei: implement arch_apei_get_mem_attributes()
  acpi, apei: use appropriate pgprot_t to map GHES memory

 arch/arm64/include/asm/acpi.h    | 26 ++++++++++++++++++++++++++
 arch/arm64/include/asm/memory.h  |  1 +
 arch/arm64/include/asm/pgtable.h |  2 ++
 arch/arm64/mm/proc.S             |  4 +++-
 arch/x86/kernel/acpi/apei.c      | 19 +++++++++++++++++++
 arch/x86/platform/efi/efi.c      | 18 ------------------
 drivers/acpi/apei/ghes.c         |  6 ++++--
 drivers/firmware/efi/efi.c       | 31 +++++++++++++++++++++++++++++++
 include/acpi/apei.h              |  1 +
 9 files changed, 87 insertions(+), 21 deletions(-)

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

[PATCH V9 5/5] acpi, apei: use appropriate pgprot_t 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. 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).

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 b979b5dbe5bc..98609b404dae 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -173,8 +173,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

Re: [PATCH V9 0/5] map GHES memory region according to EFI memory map

[Will Deacon]

Hi Jonathan,

On Thu, Jul 30, 2015 at 10:35:04PM +0100, Jonathan (Zhixiong) Zhang wrote:
> From: "Jonathan (Zhixiong) Zhang" <zjzhang@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.
> 
> V9:
> 1. Rebased to arm64-upstream-14543 of arm64/master.
> 2. Match strict MM type in arch_apei_get_mem_attribute().

I guess this is all going via Matt's tree? I'm happy to take the new
memory type in arch/arm64/ if there's nothing currently queued, but I
suspect it makes more sense for it to stay together.

Will

Re: [PATCH V9 0/5] map GHES memory region according to EFI memory map

[Matt Fleming]

On Mon, 03 Aug, at 12:26:58PM, Will Deacon wrote:
> 
> I guess this is all going via Matt's tree? I'm happy to take the new
> memory type in arch/arm64/ if there's nothing currently queued, but I
> suspect it makes more sense for it to stay together.

Right, it probably does make sense to take this through a single tree.

I'm still waiting for the ACPI folks to chime in on whether this series
makes sense, and in particular I'd like to hear from people that have
touched ghes.c in the past.

Rafael, Boris?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V9 0/5] map GHES memory region according to EFI memory map

[Borislav Petkov]

On Mon, Aug 03, 2015 at 05:23:54PM +0100, Matt Fleming wrote:
> Rafael, Boris?

The ghes.c change looks fine I guess. The whole patchset makes sense
now, with the arch bits extracted. So

Acked-by: Borislav Petkov <bp@suse.de>

However, we probably should work towards adhering to EFI memory
attributes on x86, long term, as we talked. But that's a future thing.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH V9 0/5] map GHES memory region according to EFI memory map

[Zhang, Jonathan Zhixiong]

On 8/3/2015 9:25 PM, Borislav Petkov wrote:
> On Mon, Aug 03, 2015 at 05:23:54PM +0100, Matt Fleming wrote:
>> Rafael, Boris?
>
> The ghes.c change looks fine I guess. The whole patchset makes sense
> now, with the arch bits extracted. So
>
> Acked-by: Borislav Petkov <bp@suse.de>
Thank you Boris/Matt/Will for the support.

>
> However, we probably should work towards adhering to EFI memory
> attributes on x86, long term, as we talked. But that's a future thing.
>
> Thanks.
>

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

Re: [PATCH V9 0/5] map GHES memory region according to EFI memory map

[Matt Fleming]

On Tue, 04 Aug, at 08:41:36AM, Zhang, Jonathan Zhixiong wrote:
> 
> On 8/3/2015 9:25 PM, Borislav Petkov wrote:
> >On Mon, Aug 03, 2015 at 05:23:54PM +0100, Matt Fleming wrote:
> >>Rafael, Boris?
> >
> >The ghes.c change looks fine I guess. The whole patchset makes sense
> >now, with the arch bits extracted. So
> >
> >Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
> Thank you Boris/Matt/Will for the support.

Jonathan, if you fixup the compiler error in PATCH 2 I'll take this
series through the EFI tree.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V9 0/5] map GHES memory region according to EFI memory map

[Matt Fleming]

On Tue, 04 Aug, at 06:25:52AM, Borislav Petkov wrote:
> On Mon, Aug 03, 2015 at 05:23:54PM +0100, Matt Fleming wrote:
> > Rafael, Boris?
> 
> The ghes.c change looks fine I guess. The whole patchset makes sense
> now, with the arch bits extracted. So
> 
> Acked-by: Borislav Petkov <bp@suse.de>
 
Thanks Boris.

> However, we probably should work towards adhering to EFI memory
> attributes on x86, long term, as we talked. But that's a future thing.

Yeah, it's on my TODO list.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V9 0/5] map GHES memory region according to EFI memory map

[Zhang, Jonathan Zhixiong]

On 8/5/2015 2:21 AM, Matt Fleming wrote:
> On Tue, 04 Aug, at 08:41:36AM, Zhang, Jonathan Zhixiong wrote:
>>
>> On 8/3/2015 9:25 PM, Borislav Petkov wrote:
>>> On Mon, Aug 03, 2015 at 05:23:54PM +0100, Matt Fleming wrote:
>>>> Rafael, Boris?
>>>
>>> The ghes.c change looks fine I guess. The whole patchset makes sense
>>> now, with the arch bits extracted. So
>>>
>>> Acked-by: Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>
>> Thank you Boris/Matt/Will for the support.
>
> Jonathan, if you fixup the compiler error in PATCH 2 I'll take this
> series through the EFI tree.
Yes, absolutely. It has been in my mind, sorry I was tied up in the last
few days. I wonder what I may missed, I certainly do x86 build test with
versions of the patch set.

I will try today and report back.

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

Re: [PATCH V9 0/5] map GHES memory region according to EFI memory map

[Matt Fleming]

On Wed, 05 Aug, at 08:58:52AM, Zhang, Jonathan Zhixiong wrote:
> 
> Yes, absolutely. It has been in my mind, sorry I was tied up in the last
> few days. I wonder what I may missed, I certainly do x86 build test with
> versions of the patch set.
> 
> I will try today and report back.

No problem.

I'd suggest moving the implementation into include/acpi/apei.h and
leaving it as static inline instead of implementing it out of line in
arch/x86/kernel/acpi/apei.c.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V9 0/5] map GHES memory region according to EFI memory map

[Matt Fleming]

On Wed, 05 Aug, at 05:10:00PM, Matt Fleming wrote:
> On Wed, 05 Aug, at 08:58:52AM, Zhang, Jonathan Zhixiong wrote:
> > 
> > Yes, absolutely. It has been in my mind, sorry I was tied up in the last
> > few days. I wonder what I may missed, I certainly do x86 build test with
> > versions of the patch set.
> > 
> > I will try today and report back.
> 
> No problem.
> 
> I'd suggest moving the implementation into include/acpi/apei.h and
> leaving it as static inline instead of implementing it out of line in
> arch/x86/kernel/acpi/apei.c.

Yeah, I didn't mean include/acpi/apei.h but the architecture-specific
equivalent.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH V9 0/5] map GHES memory region according to EFI memory map

[Zhang, Jonathan Zhixiong]

On 8/5/2015 9:13 AM, Matt Fleming wrote:
> On Wed, 05 Aug, at 05:10:00PM, Matt Fleming wrote:
>> On Wed, 05 Aug, at 08:58:52AM, Zhang, Jonathan Zhixiong wrote:
>>>
>>> Yes, absolutely. It has been in my mind, sorry I was tied up in the last
>>> few days. I wonder what I may missed, I certainly do x86 build test with
>>> versions of the patch set.
>>>
>>> I will try today and report back.
>>
>> No problem.
>>
>> I'd suggest moving the implementation into include/acpi/apei.h and
>> leaving it as static inline instead of implementing it out of line in
>> arch/x86/kernel/acpi/apei.c.
>
> Yeah, I didn't mean include/acpi/apei.h but the architecture-specific
> equivalent.
>
Thank you Matt. It builds well on x86 with default configuration, but
not if APEI feature is turned on. This is my fault.
I just sent a new version of the patch set out.

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