From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Kiper Subject: Re: [PATCH v5 1/7] efi: Use early_mem*() instead of early_io*() Date: Wed, 18 Jun 2014 14:59:57 +0200 Message-ID: <20140618125957.GA28489__10698.1274708689$1403096665$gmane$org@olila.local.net-space.pl> References: <1402678823-24589-1-git-send-email-daniel.kiper@oracle.com> <1402678823-24589-2-git-send-email-daniel.kiper@oracle.com> <20140618121709.GF24049@console-pimps.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WxFVR-0007v1-31 for xen-devel@lists.xenproject.org; Wed, 18 Jun 2014 13:02:25 +0000 Content-Disposition: inline In-Reply-To: <20140618121709.GF24049@console-pimps.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Matt Fleming Cc: mjg59@srcf.ucam.org, jeremy@goop.org, linux-efi@vger.kernel.org, Mark Salter , ian.campbell@citrix.com, andrew.cooper3@citrix.com, x86@kernel.org, linux-kernel@vger.kernel.org, matt.fleming@intel.com, tglx@linutronix.de, david.vrabel@citrix.com, jbeulich@suse.com, hpa@zytor.com, xen-devel@lists.xenproject.org, boris.ostrovsky@oracle.com, mingo@redhat.com, stefano.stabellini@eu.citrix.com, eshelton@pobox.com List-Id: xen-devel@lists.xenproject.org On Wed, Jun 18, 2014 at 01:17:09PM +0100, Matt Fleming wrote: > (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 > > --- > > 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. I think that it improves readability a bit but if you wish I will not do that in the future. > As Matthew pointed out we may also need to access EFI mapped flash > devices. Right, but I think it does not change a lot in that case. Flash is simply slower type of memory used as permanent storage. Am I missing something? > 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? As I saw David provided better explanation. If you wish I could include it in commit message. Daniel