From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932617AbcBZIht (ORCPT ); Fri, 26 Feb 2016 03:37:49 -0500 Received: from mx2.suse.de ([195.135.220.15]:34789 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753850AbcBZIhr (ORCPT ); Fri, 26 Feb 2016 03:37:47 -0500 Date: Fri, 26 Feb 2016 09:37:41 +0100 From: Borislav Petkov To: Alexander Kuleshov Cc: Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , x86@kernel.org, Joerg Roedel , Dave Young , Andrew Morton , Baoquan He , Mark Salter , linux-kernel@vger.kernel.org Subject: Re: [PATCH v7] x86/setup: get ramdisk parameters only once Message-ID: <20160226083741.GA12729@pd.tnic> References: <1456471905-28534-1-git-send-email-kuleshovmail@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1456471905-28534-1-git-send-email-kuleshovmail@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Feb 26, 2016 at 01:31:45PM +0600, Alexander Kuleshov wrote: > arch/x86/kernel/setup.c | 109 ++++++++++++++++++++++++++---------------------- > 1 file changed, 60 insertions(+), 49 deletions(-) > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index d3d80e6..449b4da 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -169,6 +169,15 @@ static struct resource bss_resource = { > .flags = IORESOURCE_BUSY | IORESOURCE_MEM > }; > > +/* > + * ramdisk setup > + */ No need for that comment - the struct naming and members should be sufficient. > +struct ramdisk { > + u64 start_addr; /* ramdisk load address */ > + u64 end_addr; /* ramdisk end address */ > + u64 size; /* ramdisk size */ > + bool reserve_ramdisk; /* do initrd provided by bootloader */ > +}; > > #ifdef CONFIG_X86_32 > /* cpu data as detected by the assembly code in head.S */ > @@ -318,90 +327,84 @@ static u64 __init get_ramdisk_size(void) > return ramdisk_size; > } > > -static void __init relocate_initrd(void) > +static void __init relocate_initrd(struct ramdisk *ramdisk) > { > - /* Assume only end is not page aligned */ > - u64 ramdisk_image = get_ramdisk_image(); > - u64 ramdisk_size = get_ramdisk_size(); > - u64 area_size = PAGE_ALIGN(ramdisk_size); > - > /* We need to move the initrd down into directly mapped mem */ > relocated_ramdisk = memblock_find_in_range(0, PFN_PHYS(max_pfn_mapped), > - area_size, PAGE_SIZE); > + ramdisk->size, PAGE_SIZE); > > if (!relocated_ramdisk) > panic("Cannot find place for new RAMDISK of size %lld\n", > - ramdisk_size); > + ramdisk->size); > > /* Note: this includes all the mem currently occupied by > the initrd, we rely on that fact to keep the data intact. */ > - memblock_reserve(relocated_ramdisk, area_size); > + memblock_reserve(relocated_ramdisk, ramdisk->size); > initrd_start = relocated_ramdisk + PAGE_OFFSET; > - initrd_end = initrd_start + ramdisk_size; > + initrd_end = initrd_start + ramdisk->size; > printk(KERN_INFO "Allocated new RAMDISK: [mem %#010llx-%#010llx]\n", I think all those printks talking about ramdisk should be printk(KERN_INFO "RAMDISK: ..." for easier grepping of dmesg about ramdisk-specific messages. This should be another patch though. > - relocated_ramdisk, relocated_ramdisk + ramdisk_size - 1); > + relocated_ramdisk, relocated_ramdisk + ramdisk->size - 1); > > - copy_from_early_mem((void *)initrd_start, ramdisk_image, ramdisk_size); > + copy_from_early_mem((void *)initrd_start, ramdisk->start_addr, ramdisk->size); > > printk(KERN_INFO "Move RAMDISK from [mem %#010llx-%#010llx] to" > " [mem %#010llx-%#010llx]\n", > - ramdisk_image, ramdisk_image + ramdisk_size - 1, > - relocated_ramdisk, relocated_ramdisk + ramdisk_size - 1); > + ramdisk->start_addr, ramdisk->start_addr + ramdisk->size - 1, > + relocated_ramdisk, relocated_ramdisk + ramdisk->size - 1); > } > - That \n should not have been deleted here. > -static void __init early_reserve_initrd(void) > +static void __init early_reserve_initrd(struct ramdisk *ramdisk) > { > - /* Assume only end is not page aligned */ > - u64 ramdisk_image = get_ramdisk_image(); > - u64 ramdisk_size = get_ramdisk_size(); > - u64 ramdisk_end = PAGE_ALIGN(ramdisk_image + ramdisk_size); > - > - if (!boot_params.hdr.type_of_loader || > - !ramdisk_image || !ramdisk_size) > - return; /* No initrd provided by bootloader */ > - > - memblock_reserve(ramdisk_image, ramdisk_end - ramdisk_image); > + if (!boot_params.hdr.type_of_loader || !ramdisk->start_addr || !ramdisk->size) > + ramdisk->reserve_ramdisk = false; /* No initrd provided by bootloader */ > + else > + memblock_reserve(ramdisk->start_addr, ramdisk->size); > } Make this one even more readable: static void __init early_reserve_initrd(struct ramdisk *ramdisk) { /* No initrd provided by bootloader */ if (!boot_params.hdr.type_of_loader || !ramdisk->start_addr || !ramdisk->size) ramdisk->reserve_ramdisk = false; else memblock_reserve(ramdisk->start_addr, ramdisk->size); } It also has some whitespace damage. > -static void __init reserve_initrd(void) > + > +static void __init reserve_initrd(struct ramdisk *ramdisk) > { > - /* Assume only end is not page aligned */ > - u64 ramdisk_image = get_ramdisk_image(); > - u64 ramdisk_size = get_ramdisk_size(); > - u64 ramdisk_end = PAGE_ALIGN(ramdisk_image + ramdisk_size); > u64 mapped_size; > > - if (!boot_params.hdr.type_of_loader || > - !ramdisk_image || !ramdisk_size) > - return; /* No initrd provided by bootloader */ > + if (!ramdisk->reserve_ramdisk) > + return; > > initrd_start = 0; > > mapped_size = memblock_mem_size(max_pfn_mapped); > - if (ramdisk_size >= (mapped_size>>1)) > + if (ramdisk->size >= (mapped_size>>1)) Space that shift out: ... mapped_size >> 1)) > panic("initrd too large to handle, " > "disabling initrd (%lld needed, %lld available)\n", The string in this panic() call should be a single line only for easier grepping. With another patch though. > - ramdisk_size, mapped_size>>1); > + ramdisk->size, mapped_size>>1); Space that shift out too. > > - printk(KERN_INFO "RAMDISK: [mem %#010llx-%#010llx]\n", ramdisk_image, > - ramdisk_end - 1); > + printk(KERN_INFO "RAMDISK: [mem %#010llx-%#010llx]\n", > + ramdisk->start_addr, ramdisk->end_addr - 1); > > - if (pfn_range_is_mapped(PFN_DOWN(ramdisk_image), > - PFN_DOWN(ramdisk_end))) { > + if (pfn_range_is_mapped(PFN_DOWN(ramdisk->start_addr), > + PFN_DOWN(ramdisk->end_addr))) { > /* All are mapped, easy case */ > - initrd_start = ramdisk_image + PAGE_OFFSET; > - initrd_end = initrd_start + ramdisk_size; > + initrd_start = ramdisk->start_addr + PAGE_OFFSET; > + initrd_end = initrd_start + ramdisk->size; > return; > } > > - relocate_initrd(); > + relocate_initrd(ramdisk); > > - memblock_free(ramdisk_image, ramdisk_end - ramdisk_image); > + memblock_free(ramdisk->start_addr, > + ramdisk->end_addr - ramdisk->start_addr); Arg alignment should start at the function's opening brace. > } > #else > -static void __init early_reserve_initrd(void) > +static u64 __init get_ramdisk_image(void) > { > + return 0; > } > -static void __init reserve_initrd(void) > +static u64 __init get_ramdisk_size(void) > +{ > + return 0; > +} > +static void __init early_reserve_initrd(struct ramdisk *ramdisk) > +{ > + > +} > +static void __init reserve_initrd(struct ramdisk *ramdisk) > { > } > #endif /* CONFIG_BLK_DEV_INITRD */ > @@ -844,13 +847,21 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p) > * > * Note: On x86_64, fixmaps are ready for use even before this is called. > */ > - > void __init setup_arch(char **cmdline_p) > { > + struct ramdisk ramdisk = { > + .start_addr = get_ramdisk_image(), > + .size = PAGE_ALIGN(get_ramdisk_size()), > + .reserve_ramdisk = true > + }; More readable: struct ramdisk ramdisk = { .start_addr = get_ramdisk_image(), .size = PAGE_ALIGN(get_ramdisk_size()), .reserve_ramdisk = true, }; > + > + /* Assume end is not page aligned */ > + ramdisk.end_addr = PAGE_ALIGN(ramdisk.start_addr + ramdisk.size); > + > memblock_reserve(__pa_symbol(_text), > (unsigned long)__bss_stop - (unsigned long)_text); > > - early_reserve_initrd(); > + early_reserve_initrd(&ramdisk); > > /* > * At this point everything still needed from the boot loader > @@ -1135,7 +1146,7 @@ void __init setup_arch(char **cmdline_p) > /* Allocate bigger log buffer */ > setup_log_buf(1); > > - reserve_initrd(); > + reserve_initrd(&ramdisk); > > #if defined(CONFIG_ACPI) && defined(CONFIG_BLK_DEV_INITRD) > acpi_initrd_override((void *)initrd_start, initrd_end - initrd_start); > -- > 2.7.2.335.g3476f70 > -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --