linux-kernel - Re: [PATCH V5 3/4] arm64: apei: implement arch_apei_get_mem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 16 Jul 2015 18:37:00 -0700
From:	"Zhang, Jonathan Zhixiong" <zjzhang@...eaurora.org>
To:	Will Deacon <will.deacon@....com>
CC:	Catalin Marinas <Catalin.Marinas@....com>,
	"fu.wei@...aro.org" <fu.wei@...aro.org>,
	"al.stone@...aro.org" <al.stone@...aro.org>,
	"bp @ alien8 . de Matt Fleming" <matt.fleming@...el.com>,
	"rjw@...ysocki.net" <rjw@...ysocki.net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linaro-acpi@...ts.linaro.org" <linaro-acpi@...ts.linaro.org>
Subject: Re: [PATCH V5 3/4] arm64: apei: implement arch_apei_get_mem_attributes()

Thank you Will for the feedback. Pls. see comments below.

On 7/16/2015 10:18 AM, Will Deacon wrote:
> On Wed, Jul 15, 2015 at 01:31:55AM +0100, Jonathan (Zhixiong) Zhang wrote:
>> From: "Jonathan (Zhixiong) Zhang" <zjzhang@...eaurora.org>
>>
>> If the physical address has memory attributes defined by EFI
>> memmap as EFI_MEMORY_UC, the page protection type is
>> PROT_DEVICE_nGnRE. Otherwise, the page protection type is
>> PAGE_KERNEL.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@...eaurora.org>
>> ---
>>   arch/arm64/kernel/Makefile |  1 +
>>   arch/arm64/kernel/apei.c   | 27 +++++++++++++++++++++++++++
>>   2 files changed, 28 insertions(+)
>>   create mode 100644 arch/arm64/kernel/apei.c
>>
>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>> index 426d0763c81b..c135f1e4e3b8 100644
>> --- a/arch/arm64/kernel/Makefile
>> +++ b/arch/arm64/kernel/Makefile
>> @@ -36,6 +36,7 @@ arm64-obj-$(CONFIG_EFI)			+= efi.o efi-stub.o efi-entry.o
>>   arm64-obj-$(CONFIG_PCI)			+= pci.o
>>   arm64-obj-$(CONFIG_ARMV8_DEPRECATED)	+= armv8_deprecated.o
>>   arm64-obj-$(CONFIG_ACPI)		+= acpi.o
>> +arm64-obj-$(CONFIG_ACPI_APEI)		+= apei.o
>>
>>   obj-y					+= $(arm64-obj-y) vdso/
>>   obj-m					+= $(arm64-obj-m)
>> diff --git a/arch/arm64/kernel/apei.c b/arch/arm64/kernel/apei.c
>> new file mode 100644
>> index 000000000000..9c6b62f40ad7
>> --- /dev/null
>> +++ b/arch/arm64/kernel/apei.c
>> @@ -0,0 +1,27 @@
>> +/*
>> + * Arch-specific APEI-related functions.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <acpi/apei.h>
>> +
>> +#include <linux/efi.h>
>> +
>> +#include <asm/pgtable.h>
>> +
>> +pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>> +{
>> +	if (efi_mem_attributes(addr) & EFI_MEMORY_UC)
>> +		return PROT_DEVICE_nGnRE;
>> +	else
>> +		return PAGE_KERNEL;
>> +}
>
> Do we really need a new file and out-of-line call for this?
We have a choice of either adding this function to
arch/arm64/kernel/acpi.c, or creating
arch/arm64/kernel/apei.c. As we continue to work on firmware first
HW error handling for arm64, more arm64 specific APEI related functions
may need to be implemented, thus I think it would be good to create
arch/arm64/kernel/apei.c. That being said, to date we have found
the needs to have only two arm64 specific APEI related functions.
The other one can be found in LEG kernel, through this commit:
   aa2d69c88b27 ACPI, APEI, ARM64: APEI initial support for aarch64
My understanding is that Linaro will work on to upstream that commit. I
do not strongly prefer either choice.

When APEI ghes driver maps the memory region that has error record
updated by firmware, it executes in IRQ, timer or SEA handler. Since
ioremap() can not be used in atomic context, so APEI implements a
special version of atomic ioremap function calling ioremap_page_range().
On the other hand, x86 and ARM64 have different ways to define pgprot_t
for page that needs to be accessed with uncached property. x86 defines
PAGE_KERNEL_NOCACHE, while arm64 defines PROT_DEVICE_nGnRE. Therefore
arch specific implementation is needed.
There are other ways to achieve such needs. V3 of this
patch set tried another way [1]. I think the current way makes the most
sense, since it made generic APEI code to stay generic (no knowledge
about EFI, no arch dependent ifdefs).

There are discussions centering around making this memory attribute
retrieval generic. As Matt said in [2]: APEI case is special because
irrespective of what the kernel says we want to be compatible with the
firmware's memory map.

[1] 
http://fa.linux.kernel.narkive.com/YYXPJIjs/patch-v3-0-4-map-ghes-memory-region-with-efi-memory-map
[2] 
http://fa.linux.kernel.narkive.com/YYXPJIjs/patch-v3-0-4-map-ghes-memory-region-with-efi-memory-map


>
> Will
>

-- 
Jonathan (Zhixiong) Zhang
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/