Re: [PATCH 1/2] efi: arch, x86: arch, ia64: rearrange EFI memmap related functions

["Zhang, Jonathan Zhixiong" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

Thank you Matt very much for the code review! Pls. see comments inline.

On 5/27/2015 5:31 AM, Matt Fleming wrote:
> On Mon, 04 May, at 02:02:14PM, Jonathan (Zhixiong) Zhang wrote:
>> From: "Jonathan (Zhixiong) Zhang" <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>>
>> Both x86 and ia64 implemented efi_mem_attributs function, which is architecture
>> agnositc. This function is moved to efi subsystem.
>>
>> efi_remap() function is added. If EFI memmap feature is enabled, and if a
>> memory region has attribute of EFI_MEMORY_UC, map it as uncached.
>>
>> ---
>> This patch was tested on an arm64 platform. It was built on x86 platform.
>>
>> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>> ---
>>   arch/ia64/kernel/efi.c      | 11 -----------
>>   arch/x86/platform/efi/efi.c | 18 ------------------
>>   drivers/firmware/efi/efi.c  | 27 +++++++++++++++++++++++++++
>>   include/linux/efi.h         |  1 +
>>   4 files changed, 28 insertions(+), 29 deletions(-)
>>
 >> <snipped>
>
> This should be split into two patches, one to remove the duplicate
> efi_mem_attributes() and the other to create the new efi_ioremap()
> function.
Makes sense, will do.

>
>> +void __iomem *efi_remap(phys_addr_t phys_addr, size_t size)
>> +{
>> +	if (efi_enabled(EFI_MEMMAP) &&
>> +	    (efi_mem_attributes(phys_addr) & EFI_MEMORY_UC))
>> +		return ioremap(phys_addr, size);
>> +	else
>> +		return ioremap_cache(phys_addr, size);
>> +}
>
> Note that on x86 we don't leave the EFI memmap mapped throughout
> runtime, it gets unmapped in efi_free_boot_services().
>
> Which means that the second patch in this series isn't going to work
> correctly if an error is reported after the kernel has finished booting.
>
> It looks like arm64 leaves the EFI memmap mapped at runtime, right?
>
Correct, only x86 unmaps EFI memmap since efi_free_boot_services() is 
not implemented for other architectures, as shown in include/linux/efi.h .

With x86 arch, efi_unmap_memmap() is called in efi_free_boot_services() 
to unmap EFI memmap, this function is defined in 
arch/x86/platform/efi/efi.c. It clears EFI_MEMMAP bit in efi flag. 
Therefore in this case, efi_enabled(EFI_MEMMAP) returns false for x86 
platforms with EFI memory map support, after kernel finished booting; 
efi_remap() function in turn maps the memory region as cached.

Therefore, I believe the second patch in this series will work correctly 
for above situation as well. How do you think?

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