Re: [PATCH V5 3/4] arm64: apei: implement arch_apei_get_mem_attributes()

[Hanjun Guo <hanjun.guo@linaro.org>]

On 07/17/2015 09:37 AM, Zhang, Jonathan Zhixiong wrote:
> 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@codeaurora.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@codeaurora.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.

For me, I would like to locate those codes in arm64/kernel/acpi.c and
guard with CONFIG_ACPI_APEI now, if we want to extend in the future,
then let's see if we do anything for that, in this way, we can make ACPI
on ARM64 (the arch code) self-contained.

Thanks
Hanjun