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

[Zhang, Jonathan Zhixiong]

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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/