* [PATCH v7] x86/setup: get ramdisk parameters only once
@ 2016-02-26 7:31 Alexander Kuleshov
2016-02-26 8:37 ` Borislav Petkov
0 siblings, 1 reply; 4+ messages in thread
From: Alexander Kuleshov @ 2016-02-26 7:31 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, H . Peter Anvin, x86, Borislav Petkov, Joerg Roedel,
Dave Young, Andrew Morton, Baoquan He, Mark Salter, linux-kernel,
Alexander Kuleshov
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v7] x86/setup: get ramdisk parameters only once
2016-02-26 7:31 [PATCH v7] x86/setup: get ramdisk parameters only once Alexander Kuleshov
@ 2016-02-26 8:37 ` Borislav Petkov
2016-02-27 11:55 ` Ingo Molnar
0 siblings, 1 reply; 4+ messages in thread
From: Borislav Petkov @ 2016-02-26 8:37 UTC (permalink / raw)
To: Alexander Kuleshov
Cc: Thomas Gleixner, Ingo Molnar, H . Peter Anvin, x86, Joerg Roedel,
Dave Young, Andrew Morton, Baoquan He, Mark Salter, linux-kernel
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)
--
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v7] x86/setup: get ramdisk parameters only once
2016-02-26 8:37 ` Borislav Petkov
@ 2016-02-27 11:55 ` Ingo Molnar
2016-02-27 12:44 ` Borislav Petkov
0 siblings, 1 reply; 4+ messages in thread
From: Ingo Molnar @ 2016-02-27 11:55 UTC (permalink / raw)
To: Borislav Petkov
Cc: Alexander Kuleshov, Thomas Gleixner, Ingo Molnar, H . Peter Anvin,
x86, Joerg Roedel, Dave Young, Andrew Morton, Baoquan He,
Mark Salter, linux-kernel
* 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v7] x86/setup: get ramdisk parameters only once
2016-02-27 11:55 ` Ingo Molnar
@ 2016-02-27 12:44 ` Borislav Petkov
0 siblings, 0 replies; 4+ messages in thread
From: Borislav Petkov @ 2016-02-27 12:44 UTC (permalink / raw)
To: Ingo Molnar
Cc: Alexander Kuleshov, Thomas Gleixner, Ingo Molnar, H . Peter Anvin,
x86, Joerg Roedel, Dave Young, Andrew Morton, Baoquan He,
Mark Salter, linux-kernel
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)
--
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-27 12:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-26 7:31 [PATCH v7] x86/setup: get ramdisk parameters only once Alexander Kuleshov
2016-02-26 8:37 ` Borislav Petkov
2016-02-27 11:55 ` Ingo Molnar
2016-02-27 12:44 ` Borislav Petkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox