Messages in this thread | | | Date | Wed, 27 May 2015 12:35:49 -0700 | From | "Zhang, Jonathan Zhixiong" <> | Subject | Re: [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
| |