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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 27 May 2015 12:35:49 -0700
From:	"Zhang, Jonathan Zhixiong" <zjzhang@...eaurora.org>
To:	Matt Fleming <matt@...eblueprint.co.uk>
CC:	Matt Fleming <matt.fleming@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, linaro-acpi@...ts.linaro.org
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@...eaurora.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@...eaurora.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/