From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754124AbbGXCH6 (ORCPT ); Thu, 23 Jul 2015 22:07:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51100 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753404AbbGXCHz (ORCPT ); Thu, 23 Jul 2015 22:07:55 -0400 Date: Fri, 24 Jul 2015 10:07:48 +0800 From: Dave Young To: Yinghai Lu Cc: Kees Cook , "H. Peter Anvin" , Baoquan He , linux-kernel@vger.kernel.org, Matt Fleming , linux-efi@vger.kernel.org Subject: Re: [PATCH 31/42] x86, efi: Copy SETUP_EFI data and access directly Message-ID: <20150724020748.GB5508@dhcp-128-11.nay.redhat.com> References: <1436300428-21163-1-git-send-email-yinghai@kernel.org> <1436300428-21163-32-git-send-email-yinghai@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1436300428-21163-32-git-send-email-yinghai@kernel.org> User-Agent: Mutt/1.5.22.1-rc1 (2013-10-16) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 07/07/15 at 01:20pm, Yinghai Lu wrote: > The copy will be in __initdata, and it is small. > > We can use pointer to access the setup_data instead of using early_memmap > everywhere. Looks good to me except one issue about missing checking memremap return value. see the comment inline > > Cc: Matt Fleming > Cc: linux-efi@vger.kernel.org > Signed-off-by: Yinghai Lu > --- [snip] > --- a/arch/x86/platform/efi/efi_64.c > +++ b/arch/x86/platform/efi/efi_64.c > @@ -295,9 +295,17 @@ void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size, > return (void __iomem *)__va(phys_addr); > } > > +static struct efi_setup_data efi_setup_data __initdata; > + > void __init parse_efi_setup(u64 phys_addr, u32 data_len) > { > - efi_setup = phys_addr + sizeof(struct setup_data); > + struct efi_setup_data *data; > + > + data = early_memremap(phys_addr + sizeof(struct setup_data), > + sizeof(*data)); There should be a checking for return value here.. > + efi_setup_data = *data; > + early_memunmap(data, sizeof(*data)); > + efi_setup = &efi_setup_data; > } > [snip] Thanks Dave From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Young Subject: Re: [PATCH 31/42] x86, efi: Copy SETUP_EFI data and access directly Date: Fri, 24 Jul 2015 10:07:48 +0800 Message-ID: <20150724020748.GB5508@dhcp-128-11.nay.redhat.com> References: <1436300428-21163-1-git-send-email-yinghai@kernel.org> <1436300428-21163-32-git-send-email-yinghai@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1436300428-21163-32-git-send-email-yinghai-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Yinghai Lu Cc: Kees Cook , "H. Peter Anvin" , Baoquan He , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Matt Fleming , linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-efi@vger.kernel.org Hi, On 07/07/15 at 01:20pm, Yinghai Lu wrote: > The copy will be in __initdata, and it is small. > > We can use pointer to access the setup_data instead of using early_memmap > everywhere. Looks good to me except one issue about missing checking memremap return value. see the comment inline > > Cc: Matt Fleming > Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Signed-off-by: Yinghai Lu > --- [snip] > --- a/arch/x86/platform/efi/efi_64.c > +++ b/arch/x86/platform/efi/efi_64.c > @@ -295,9 +295,17 @@ void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size, > return (void __iomem *)__va(phys_addr); > } > > +static struct efi_setup_data efi_setup_data __initdata; > + > void __init parse_efi_setup(u64 phys_addr, u32 data_len) > { > - efi_setup = phys_addr + sizeof(struct setup_data); > + struct efi_setup_data *data; > + > + data = early_memremap(phys_addr + sizeof(struct setup_data), > + sizeof(*data)); There should be a checking for return value here.. > + efi_setup_data = *data; > + early_memunmap(data, sizeof(*data)); > + efi_setup = &efi_setup_data; > } > [snip] Thanks Dave