From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750964AbdALQUr (ORCPT ); Thu, 12 Jan 2017 11:20:47 -0500 Received: from mail-it0-f48.google.com ([209.85.214.48]:35818 "EHLO mail-it0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750810AbdALQUn (ORCPT ); Thu, 12 Jan 2017 11:20:43 -0500 MIME-Version: 1.0 In-Reply-To: <20170112094214.909117564@redhat.com> References: <20170112094118.815108042@redhat.com> <20170112094214.909117564@redhat.com> From: Ard Biesheuvel Date: Thu, 12 Jan 2017 16:20:41 +0000 Message-ID: Subject: Re: [PATCH 2/4] efi/x86: move efi bgrt init code to early init code To: Dave Young Cc: Matt Fleming , "linux-efi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "x86@kernel.org" , Nicolai Stange , Ingo Molnar , Thomas Gleixner , "hpa@zytor.com" , Dan Williams , =?UTF-8?Q?Mika_Penttil=C3=A4?= , bhsharma@redhat.com Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12 January 2017 at 09:41, Dave Young wrote: > Before invoking the arch specific handler, efi_mem_reserve() reserves > the given memory region through memblock. > > efi_bgrt_init will call efi_mem_reserve after mm_init(), at that time > memblock is dead and it should not be used any more. > > efi bgrt code depend on acpi intialization to get the bgrt acpi table, > moving bgrt parsing to acpi early boot code can make sure efi_mem_reserve > in efi bgrt code still use memblock safely. > > Signed-off-by: Dave Young I know this is probably out of scope for you, but since we're moving things around, any chance we could do so in a manner that will enable BGRT support for arm64/ACPI? Happy to test/collaborate on this. > --- > arch/x86/kernel/acpi/boot.c | 12 +++++++++++ > arch/x86/platform/efi/efi-bgrt.c | 42 +++++++++++++-------------------------- > arch/x86/platform/efi/efi.c | 5 ---- > drivers/acpi/bgrt.c | 21 +++++++++++++------ > include/linux/efi-bgrt.h | 7 ++---- > init/main.c | 1 > 6 files changed, 45 insertions(+), 43 deletions(-) > > --- linux-x86.orig/arch/x86/kernel/acpi/boot.c > +++ linux-x86/arch/x86/kernel/acpi/boot.c > @@ -35,6 +35,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -1557,6 +1558,14 @@ int __init early_acpi_boot_init(void) > return 0; > } > > +#ifdef CONFIG_ACPI_BGRT > +static int __init acpi_parse_bgrt(struct acpi_table_header *table) > +{ > + efi_bgrt_init(table); > + return 0; > +} > +#endif > + > int __init acpi_boot_init(void) > { > /* those are executed after early-quirks are executed */ > @@ -1581,6 +1590,9 @@ int __init acpi_boot_init(void) > acpi_process_madt(); > > acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet); > +#ifdef CONFIG_ACPI_BGRT > + acpi_table_parse("BGRT", acpi_parse_bgrt); > +#endif > > if (!acpi_noirq) > x86_init.pci.init = pci_acpi_init; > --- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c > +++ linux-x86/arch/x86/platform/efi/efi-bgrt.c > @@ -19,8 +19,7 @@ > #include > #include > > -struct acpi_table_bgrt *bgrt_tab; > -void *__initdata bgrt_image; > +struct acpi_table_bgrt bgrt_tab; > size_t __initdata bgrt_image_size; > > struct bmp_header { > @@ -28,53 +27,49 @@ struct bmp_header { > u32 size; > } __packed; > > -void __init efi_bgrt_init(void) > +void __init efi_bgrt_init(struct acpi_table_header *table) > { > - acpi_status status; > void *image; > struct bmp_header bmp_header; > > if (acpi_disabled) > return; > > - status = acpi_get_table("BGRT", 0, > - (struct acpi_table_header **)&bgrt_tab); > - if (ACPI_FAILURE(status)) > - return; > + bgrt_tab = *(struct acpi_table_bgrt *)table; > > - if (bgrt_tab->header.length < sizeof(*bgrt_tab)) { > + if (bgrt_tab.header.length < sizeof(bgrt_tab)) { > pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n", > - bgrt_tab->header.length, sizeof(*bgrt_tab)); > + bgrt_tab.header.length, sizeof(bgrt_tab)); > return; > } > - if (bgrt_tab->version != 1) { > + if (bgrt_tab.version != 1) { > pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n", > - bgrt_tab->version); > + bgrt_tab.version); > return; > } > - if (bgrt_tab->status & 0xfe) { > + if (bgrt_tab.status & 0xfe) { > pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n", > - bgrt_tab->status); > + bgrt_tab.status); > return; > } > - if (bgrt_tab->image_type != 0) { > + if (bgrt_tab.image_type != 0) { > pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n", > - bgrt_tab->image_type); > + bgrt_tab.image_type); > return; > } > - if (!bgrt_tab->image_address) { > + if (!bgrt_tab.image_address) { > pr_notice("Ignoring BGRT: null image address\n"); > return; > } > > - image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB); > + image = early_memremap(bgrt_tab.image_address, sizeof(bmp_header)); > if (!image) { > pr_notice("Ignoring BGRT: failed to map image header memory\n"); > return; > } > > memcpy(&bmp_header, image, sizeof(bmp_header)); > - memunmap(image); > + early_memunmap(image, sizeof(bmp_header)); > if (bmp_header.id != 0x4d42) { > pr_notice("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n", > bmp_header.id); > @@ -82,12 +77,5 @@ void __init efi_bgrt_init(void) > } > bgrt_image_size = bmp_header.size; > > - bgrt_image = memremap(bgrt_tab->image_address, bmp_header.size, MEMREMAP_WB); > - if (!bgrt_image) { > - pr_notice("Ignoring BGRT: failed to map image memory\n"); > - bgrt_image = NULL; > - return; > - } > - > - efi_mem_reserve(bgrt_tab->image_address, bgrt_image_size); > + efi_mem_reserve(bgrt_tab.image_address, bgrt_image_size); > } > --- linux-x86.orig/drivers/acpi/bgrt.c > +++ linux-x86/drivers/acpi/bgrt.c > @@ -15,40 +15,41 @@ > #include > #include > > +static void *bgrt_image; > static struct kobject *bgrt_kobj; > > static ssize_t show_version(struct device *dev, > struct device_attribute *attr, char *buf) > { > - return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->version); > + return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.version); > } > static DEVICE_ATTR(version, S_IRUGO, show_version, NULL); > > static ssize_t show_status(struct device *dev, > struct device_attribute *attr, char *buf) > { > - return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->status); > + return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status); > } > static DEVICE_ATTR(status, S_IRUGO, show_status, NULL); > > static ssize_t show_type(struct device *dev, > struct device_attribute *attr, char *buf) > { > - return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_type); > + return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_type); > } > static DEVICE_ATTR(type, S_IRUGO, show_type, NULL); > > static ssize_t show_xoffset(struct device *dev, > struct device_attribute *attr, char *buf) > { > - return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_x); > + return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_x); > } > static DEVICE_ATTR(xoffset, S_IRUGO, show_xoffset, NULL); > > static ssize_t show_yoffset(struct device *dev, > struct device_attribute *attr, char *buf) > { > - return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_y); > + return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_y); > } > static DEVICE_ATTR(yoffset, S_IRUGO, show_yoffset, NULL); > > @@ -84,9 +85,17 @@ static int __init bgrt_init(void) > { > int ret; > > - if (!bgrt_image) > + if (!bgrt_tab.image_address) > return -ENODEV; > > + bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size, > + MEMREMAP_WB); > + if (!bgrt_image) { > + pr_notice("Ignoring BGRT: failed to map image memory\n"); > + bgrt_image = NULL; > + return -ENOMEM; > + } > + > bin_attr_image.private = bgrt_image; > bin_attr_image.size = bgrt_image_size; > > --- linux-x86.orig/include/linux/efi-bgrt.h > +++ linux-x86/include/linux/efi-bgrt.h > @@ -5,16 +5,15 @@ > > #include > > -void efi_bgrt_init(void); > +void efi_bgrt_init(struct acpi_table_header *table); > > /* The BGRT data itself; only valid if bgrt_image != NULL. */ > -extern void *bgrt_image; > extern size_t bgrt_image_size; > -extern struct acpi_table_bgrt *bgrt_tab; > +extern struct acpi_table_bgrt bgrt_tab; > > #else /* !CONFIG_ACPI_BGRT */ > > -static inline void efi_bgrt_init(void) {} > +static inline void efi_bgrt_init(struct acpi_table_header *table) {} > > #endif /* !CONFIG_ACPI_BGRT */ > > --- linux-x86.orig/arch/x86/platform/efi/efi.c > +++ linux-x86/arch/x86/platform/efi/efi.c > @@ -476,11 +476,6 @@ void __init efi_init(void) > efi_print_memmap(); > } > > -void __init efi_late_init(void) > -{ > - efi_bgrt_init(); > -} > - > void __init efi_set_executable(efi_memory_desc_t *md, bool executable) > { > u64 addr, npages; > --- linux-x86.orig/init/main.c > +++ linux-x86/init/main.c > @@ -663,7 +663,6 @@ asmlinkage __visible void __init start_k > sfi_init_late(); > > if (efi_enabled(EFI_RUNTIME_SERVICES)) { > - efi_late_init(); > efi_free_boot_services(); > } > > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ard Biesheuvel Subject: Re: [PATCH 2/4] efi/x86: move efi bgrt init code to early init code Date: Thu, 12 Jan 2017 16:20:41 +0000 Message-ID: References: <20170112094118.815108042@redhat.com> <20170112094214.909117564@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <20170112094214.909117564-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Dave Young Cc: Matt Fleming , "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org" , Nicolai Stange , Ingo Molnar , Thomas Gleixner , "hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org" , Dan Williams , =?UTF-8?Q?Mika_Penttil=C3=A4?= , bhsharma-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org List-Id: linux-efi@vger.kernel.org On 12 January 2017 at 09:41, Dave Young wrote: > Before invoking the arch specific handler, efi_mem_reserve() reserves > the given memory region through memblock. > > efi_bgrt_init will call efi_mem_reserve after mm_init(), at that time > memblock is dead and it should not be used any more. > > efi bgrt code depend on acpi intialization to get the bgrt acpi table, > moving bgrt parsing to acpi early boot code can make sure efi_mem_reserve > in efi bgrt code still use memblock safely. > > Signed-off-by: Dave Young I know this is probably out of scope for you, but since we're moving things around, any chance we could do so in a manner that will enable BGRT support for arm64/ACPI? Happy to test/collaborate on this. > --- > arch/x86/kernel/acpi/boot.c | 12 +++++++++++ > arch/x86/platform/efi/efi-bgrt.c | 42 +++++++++++++-------------------------- > arch/x86/platform/efi/efi.c | 5 ---- > drivers/acpi/bgrt.c | 21 +++++++++++++------ > include/linux/efi-bgrt.h | 7 ++---- > init/main.c | 1 > 6 files changed, 45 insertions(+), 43 deletions(-) > > --- linux-x86.orig/arch/x86/kernel/acpi/boot.c > +++ linux-x86/arch/x86/kernel/acpi/boot.c > @@ -35,6 +35,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -1557,6 +1558,14 @@ int __init early_acpi_boot_init(void) > return 0; > } > > +#ifdef CONFIG_ACPI_BGRT > +static int __init acpi_parse_bgrt(struct acpi_table_header *table) > +{ > + efi_bgrt_init(table); > + return 0; > +} > +#endif > + > int __init acpi_boot_init(void) > { > /* those are executed after early-quirks are executed */ > @@ -1581,6 +1590,9 @@ int __init acpi_boot_init(void) > acpi_process_madt(); > > acpi_table_parse(ACPI_SIG_HPET, acpi_parse_hpet); > +#ifdef CONFIG_ACPI_BGRT > + acpi_table_parse("BGRT", acpi_parse_bgrt); > +#endif > > if (!acpi_noirq) > x86_init.pci.init = pci_acpi_init; > --- linux-x86.orig/arch/x86/platform/efi/efi-bgrt.c > +++ linux-x86/arch/x86/platform/efi/efi-bgrt.c > @@ -19,8 +19,7 @@ > #include > #include > > -struct acpi_table_bgrt *bgrt_tab; > -void *__initdata bgrt_image; > +struct acpi_table_bgrt bgrt_tab; > size_t __initdata bgrt_image_size; > > struct bmp_header { > @@ -28,53 +27,49 @@ struct bmp_header { > u32 size; > } __packed; > > -void __init efi_bgrt_init(void) > +void __init efi_bgrt_init(struct acpi_table_header *table) > { > - acpi_status status; > void *image; > struct bmp_header bmp_header; > > if (acpi_disabled) > return; > > - status = acpi_get_table("BGRT", 0, > - (struct acpi_table_header **)&bgrt_tab); > - if (ACPI_FAILURE(status)) > - return; > + bgrt_tab = *(struct acpi_table_bgrt *)table; > > - if (bgrt_tab->header.length < sizeof(*bgrt_tab)) { > + if (bgrt_tab.header.length < sizeof(bgrt_tab)) { > pr_notice("Ignoring BGRT: invalid length %u (expected %zu)\n", > - bgrt_tab->header.length, sizeof(*bgrt_tab)); > + bgrt_tab.header.length, sizeof(bgrt_tab)); > return; > } > - if (bgrt_tab->version != 1) { > + if (bgrt_tab.version != 1) { > pr_notice("Ignoring BGRT: invalid version %u (expected 1)\n", > - bgrt_tab->version); > + bgrt_tab.version); > return; > } > - if (bgrt_tab->status & 0xfe) { > + if (bgrt_tab.status & 0xfe) { > pr_notice("Ignoring BGRT: reserved status bits are non-zero %u\n", > - bgrt_tab->status); > + bgrt_tab.status); > return; > } > - if (bgrt_tab->image_type != 0) { > + if (bgrt_tab.image_type != 0) { > pr_notice("Ignoring BGRT: invalid image type %u (expected 0)\n", > - bgrt_tab->image_type); > + bgrt_tab.image_type); > return; > } > - if (!bgrt_tab->image_address) { > + if (!bgrt_tab.image_address) { > pr_notice("Ignoring BGRT: null image address\n"); > return; > } > > - image = memremap(bgrt_tab->image_address, sizeof(bmp_header), MEMREMAP_WB); > + image = early_memremap(bgrt_tab.image_address, sizeof(bmp_header)); > if (!image) { > pr_notice("Ignoring BGRT: failed to map image header memory\n"); > return; > } > > memcpy(&bmp_header, image, sizeof(bmp_header)); > - memunmap(image); > + early_memunmap(image, sizeof(bmp_header)); > if (bmp_header.id != 0x4d42) { > pr_notice("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n", > bmp_header.id); > @@ -82,12 +77,5 @@ void __init efi_bgrt_init(void) > } > bgrt_image_size = bmp_header.size; > > - bgrt_image = memremap(bgrt_tab->image_address, bmp_header.size, MEMREMAP_WB); > - if (!bgrt_image) { > - pr_notice("Ignoring BGRT: failed to map image memory\n"); > - bgrt_image = NULL; > - return; > - } > - > - efi_mem_reserve(bgrt_tab->image_address, bgrt_image_size); > + efi_mem_reserve(bgrt_tab.image_address, bgrt_image_size); > } > --- linux-x86.orig/drivers/acpi/bgrt.c > +++ linux-x86/drivers/acpi/bgrt.c > @@ -15,40 +15,41 @@ > #include > #include > > +static void *bgrt_image; > static struct kobject *bgrt_kobj; > > static ssize_t show_version(struct device *dev, > struct device_attribute *attr, char *buf) > { > - return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->version); > + return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.version); > } > static DEVICE_ATTR(version, S_IRUGO, show_version, NULL); > > static ssize_t show_status(struct device *dev, > struct device_attribute *attr, char *buf) > { > - return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->status); > + return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.status); > } > static DEVICE_ATTR(status, S_IRUGO, show_status, NULL); > > static ssize_t show_type(struct device *dev, > struct device_attribute *attr, char *buf) > { > - return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_type); > + return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_type); > } > static DEVICE_ATTR(type, S_IRUGO, show_type, NULL); > > static ssize_t show_xoffset(struct device *dev, > struct device_attribute *attr, char *buf) > { > - return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_x); > + return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_x); > } > static DEVICE_ATTR(xoffset, S_IRUGO, show_xoffset, NULL); > > static ssize_t show_yoffset(struct device *dev, > struct device_attribute *attr, char *buf) > { > - return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab->image_offset_y); > + return snprintf(buf, PAGE_SIZE, "%d\n", bgrt_tab.image_offset_y); > } > static DEVICE_ATTR(yoffset, S_IRUGO, show_yoffset, NULL); > > @@ -84,9 +85,17 @@ static int __init bgrt_init(void) > { > int ret; > > - if (!bgrt_image) > + if (!bgrt_tab.image_address) > return -ENODEV; > > + bgrt_image = memremap(bgrt_tab.image_address, bgrt_image_size, > + MEMREMAP_WB); > + if (!bgrt_image) { > + pr_notice("Ignoring BGRT: failed to map image memory\n"); > + bgrt_image = NULL; > + return -ENOMEM; > + } > + > bin_attr_image.private = bgrt_image; > bin_attr_image.size = bgrt_image_size; > > --- linux-x86.orig/include/linux/efi-bgrt.h > +++ linux-x86/include/linux/efi-bgrt.h > @@ -5,16 +5,15 @@ > > #include > > -void efi_bgrt_init(void); > +void efi_bgrt_init(struct acpi_table_header *table); > > /* The BGRT data itself; only valid if bgrt_image != NULL. */ > -extern void *bgrt_image; > extern size_t bgrt_image_size; > -extern struct acpi_table_bgrt *bgrt_tab; > +extern struct acpi_table_bgrt bgrt_tab; > > #else /* !CONFIG_ACPI_BGRT */ > > -static inline void efi_bgrt_init(void) {} > +static inline void efi_bgrt_init(struct acpi_table_header *table) {} > > #endif /* !CONFIG_ACPI_BGRT */ > > --- linux-x86.orig/arch/x86/platform/efi/efi.c > +++ linux-x86/arch/x86/platform/efi/efi.c > @@ -476,11 +476,6 @@ void __init efi_init(void) > efi_print_memmap(); > } > > -void __init efi_late_init(void) > -{ > - efi_bgrt_init(); > -} > - > void __init efi_set_executable(efi_memory_desc_t *md, bool executable) > { > u64 addr, npages; > --- linux-x86.orig/init/main.c > +++ linux-x86/init/main.c > @@ -663,7 +663,6 @@ asmlinkage __visible void __init start_k > sfi_init_late(); > > if (efi_enabled(EFI_RUNTIME_SERVICES)) { > - efi_late_init(); > efi_free_boot_services(); > } > > >