From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751754AbbE0Tfy (ORCPT ); Wed, 27 May 2015 15:35:54 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:51476 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751630AbbE0Tfv (ORCPT ); Wed, 27 May 2015 15:35:51 -0400 Message-ID: <55661C95.5010200@codeaurora.org> Date: Wed, 27 May 2015 12:35:49 -0700 From: "Zhang, Jonathan Zhixiong" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Matt Fleming CC: Matt Fleming , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, linaro-acpi@lists.linaro.org Subject: Re: [PATCH 1/2] efi: arch, x86: arch, ia64: rearrange EFI memmap related functions References: <1430773335-22897-1-git-send-email-zjzhang@codeaurora.org> <1430773335-22897-2-git-send-email-zjzhang@codeaurora.org> <20150527123103.GC3030@codeblueprint.co.uk> In-Reply-To: <20150527123103.GC3030@codeblueprint.co.uk> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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" >> >> 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 >> --- >> 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(-) >> >> > > 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