lkml.org 
[lkml]   [2015]   [May]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] efi: arch, x86: arch, ia64: rearrange EFI memmap related functions
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@codeaurora.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@codeaurora.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


\
 
 \ /
  Last update: 2015-05-27 22:21    [W:0.064 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site