Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1343883 > unrolled thread

[PATCH v7] x86/setup: get ramdisk parameters only once

Back to article view | Back to linux.kernel

Contents

  [PATCH v7] x86/setup: get ramdisk parameters only once Alexander Kuleshov <kuleshovmail@gmail.com> - 2016-02-26 08:40 +0100
    Re: [PATCH v7] x86/setup: get ramdisk parameters only once Borislav Petkov <bp@suse.de> - 2016-02-26 09:40 +0100
      Re: [PATCH v7] x86/setup: get ramdisk parameters only once Ingo Molnar <mingo@kernel.org> - 2016-02-27 13:00 +0100
        Re: [PATCH v7] x86/setup: get ramdisk parameters only once Borislav Petkov <bp@suse.de> - 2016-02-27 13:50 +0100

#1343883 — [PATCH v7] x86/setup: get ramdisk parameters only once

The check and definitions related to ramdisk are similar in the
early_reserve_initrd() and reserve_initrd() functions.

This patch introduces struct ramdisk which contains information
about initrd. This structure will be filled in the setup_arch()
and passed to the reserve_initrd() and relocate_initrd() functions.

This allows us to not get/check ramdisk parameters from the bootparams
every time in the early_reserve_initrd(), reserve_initrd() and
relocate_initrd() function.

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
Tested as with virtual machine as with real hardware.

Changelog:

v7:

  * ramdisk_image renamed to ramdisk in the setup_arch().

v6:

  * ramdisk_image renamed to ramdisk
  * early check of initrd and memory reservation for it is
  moved to the early_reserve_initrd() again.

v5:

  * move check of the reserve_ramdisk to the reserve_initrd()
  * align ramdisk size in setup_arch() as it maybe not page
  aligned and start_addr + size() will be different with end_addr
  in this case
  * style fixes

v4:

  * bool reserve_ramdisk moved to the struct ramdisk;
  * fields of struct ramdisk are renamed to more understandable
  names
  * early_reserve_initrd() removed as it contains only one call
  of memblock_resere. It was moved to the setup_arch
  * stubs added for get_ramdisk_image() and get_ramdisk_size()
  to prevent compilation error if CONFIG_BLK_DEV_INITRD=n
  * initialization of ramdisk->end_addr is moved from in-place
  initialization to prevent ‘ramdisk_image.size’ is used uninitialized
  in this function warning.

v3: introduced ramdisk setup which is filled in th
setup_arch() and passed to the early_reserve_initrd()
and reserve_initrd().

v2: parameter of reserve_initrd() - int -> bool.
    commit message updated.

 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
+ */
+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",
-	       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);
 }
-
-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);
 }
-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))
 		panic("initrd too large to handle, "
 		       "disabling initrd (%lld needed, %lld available)\n",
-		       ramdisk_size, mapped_size>>1);
+		       ramdisk->size, mapped_size>>1);
 
-	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);
 }
 #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
+	};
+
+	/* 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

[toc] | [next] | [standalone]

#1343909

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)
-- 

[toc] | [prev] | [next] | [standalone]

#1345000

* Borislav Petkov <bp@suse.de> wrote:

> >  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,
>         };

So I find it highly annoying that this review feedback was done by Boris, but not 
implemented in v8 :-(

I'd much have preferred fewer iterations and more careful implementation, which 
would have resulted in much less time wasted on the reviewer side...

Anyway, to stop this trainwreck I've fixed this (and a few other small details) 
and applied the patch, but I'd also like to point out that from now on I'll stop 
accepting trivial patches from serial trivial patches contributors that are not 
part of some larger, more substantial work...

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1345013

On Sat, Feb 27, 2016 at 12:55:46PM +0100, Ingo Molnar wrote:
> So I find it highly annoying that this review feedback was done by Boris, but not 
> implemented in v8 :-(

And I missed it when going over v8. :-(

FWIW, since recently I get the impression that people don't take review
feedback seriously. Starting to think, why even bother...

> I'd much have preferred fewer iterations and more careful implementation, which 
> would have resulted in much less time wasted on the reviewer side...

Agreed.

> Anyway, to stop this trainwreck I've fixed this (and a few other small details) 
> and applied the patch, but I'd also like to point out that from now on I'll stop 
> accepting trivial patches from serial trivial patches contributors that are not 
> part of some larger, more substantial work...

Good idea. ACK.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web