Re: [PATCH v5 1/7] efi: Use early_mem*() instead of early_io*()

[Matt Fleming <matt@console-pimps.org>]

(Pulling in Mark Salter for early_ioremap() knowledge)

On Fri, 13 Jun, at 07:00:17PM, Daniel Kiper wrote:
> Use early_mem*() instead of early_io*() because all mapped EFI regions
> are true RAM not I/O regions. Additionally, I/O family calls do not work
> correctly under Xen in our case. AIUI, early_io*() maps/unmaps real machine
> addresses. However, all artificial EFI structures created under Xen live
> in dom0 memory and should be mapped/unmapped using early_mem*() family
> calls which map domain memory.
> 
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  arch/x86/platform/efi/efi.c |   42 +++++++++++++++++++++---------------------
>  drivers/firmware/efi/efi.c  |    4 ++--
>  2 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 87fc96b..dd1e351 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -427,7 +427,7 @@ void __init efi_unmap_memmap(void)
>  {
>  	clear_bit(EFI_MEMMAP, &efi.flags);
>  	if (memmap.map) {
> -		early_iounmap(memmap.map, memmap.nr_map * memmap.desc_size);
> +		early_memunmap(memmap.map, memmap.nr_map * memmap.desc_size);
>  		memmap.map = NULL;
>  	}
>  }
> @@ -467,12 +467,12 @@ static int __init efi_systab_init(void *phys)
>  			if (!data)
>  				return -ENOMEM;
>  		}
> -		systab64 = early_ioremap((unsigned long)phys,
> -					 sizeof(*systab64));
> +		systab64 = early_memremap((unsigned long)phys,
> +						sizeof(*systab64));

Please don't misalign the arguments :-( This makes the diff harder to
read when all you're doing is changing the function call. You're
indentation isn't an improvement.
 
As Matthew pointed out we may also need to access EFI mapped flash
devices.

Now, the only difference between early_memremap() and early_ioremap(),
at least on x86, is PAGE_KERNEL vs. PAGE_KERNEL_IO, where PAGE_KERNEL_IO
has the additional _PAGE_BIT_IOMAP bit set in the pte. But that's a
software bit for x86.

So, even though this change should be harmless, it's not clear to me why
this change is needed. You say "I/O family calls do not work correctly
under Xen in our case", but could you provide some more explanation as
to why they don't work correctly?

-- 
Matt Fleming, Intel Open Source Technology Center