On Fri, Jul 17, 2015 at 02:37:00AM +0100, Zhang, Jonathan Zhixiong wrote:
On 7/16/2015 10:18 AM, Will Deacon wrote:
On Wed, Jul 15, 2015 at 01:31:55AM +0100, Jonathan (Zhixiong) Zhang wrote:
+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).
I understand what you're doing and my concern was much simpler than you seem to imagine. Put another way: why can't arch_apei_get_mem_attribute be a static inline in a header file (like acpi_os_ioremap in asm/acpi.h)?
Will