* Re: [Qemu-devel] [PATCH v2] arm_boot: Change initrd load address to "halfway through RAM"
2012-10-26 15:29 [Qemu-devel] [PATCH v2] arm_boot: Change initrd load address to "halfway through RAM" Peter Maydell
@ 2012-10-29 13:54 ` Aurelien Jarno
2012-10-30 13:56 ` Igor Mitsyanko
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Aurelien Jarno @ 2012-10-29 13:54 UTC (permalink / raw)
To: Peter Maydell; +Cc: Peter Crosthwaite, patches, qemu-devel, Cole Robinson
On Fri, Oct 26, 2012 at 04:29:38PM +0100, Peter Maydell wrote:
> To avoid continually having to bump the initrd load address
> to account for larger kernel images, put the initrd halfway
> through RAM. This allows large kernels on new boards with lots
> of RAM to work OK, without breaking existing usecases for
> boards with only 32MB of RAM.
>
> Note that this change fixes in passing a bug where we were
> passing an overly large max_size to load_image_targphys()
> for the initrd, which meant that we wouldn't correctly refuse
> to load an enormous initrd that didn't actually fit into RAM.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Changes v1->v2: put initrd at min(128M, ram_size / 2)
> rather than just at ram_size / 2.
>
> hw/arm-misc.h | 1 +
> hw/arm_boot.c | 40 +++++++++++++++++++++++++---------------
> 2 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
> index d02f7f0..adb1665 100644
> --- a/hw/arm-misc.h
> +++ b/hw/arm-misc.h
> @@ -56,6 +56,7 @@ struct arm_boot_info {
> const struct arm_boot_info *info);
> /* Used internally by arm_boot.c */
> int is_linux;
> + hwaddr initrd_start;
> hwaddr initrd_size;
> hwaddr entry;
> };
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index 09bf6c5..92e2cab 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -18,7 +18,6 @@
>
> #define KERNEL_ARGS_ADDR 0x100
> #define KERNEL_LOAD_ADDR 0x00010000
> -#define INITRD_LOAD_ADDR 0x00d00000
>
> /* The worlds second smallest bootloader. Set r0-r2, then jump to kernel. */
> static uint32_t bootloader[] = {
> @@ -109,7 +108,7 @@ static void set_kernel_args(const struct arm_boot_info *info)
> /* ATAG_INITRD2 */
> WRITE_WORD(p, 4);
> WRITE_WORD(p, 0x54420005);
> - WRITE_WORD(p, info->loader_start + INITRD_LOAD_ADDR);
> + WRITE_WORD(p, info->initrd_start);
> WRITE_WORD(p, initrd_size);
> }
> if (info->kernel_cmdline && *info->kernel_cmdline) {
> @@ -185,10 +184,11 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
> /* pages_in_vram */
> WRITE_WORD(p, 0);
> /* initrd_start */
> - if (initrd_size)
> - WRITE_WORD(p, info->loader_start + INITRD_LOAD_ADDR);
> - else
> + if (initrd_size) {
> + WRITE_WORD(p, info->initrd_start);
> + } else {
> WRITE_WORD(p, 0);
> + }
> /* initrd_size */
> WRITE_WORD(p, initrd_size);
> /* rd_start */
> @@ -281,14 +281,13 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>
> if (binfo->initrd_size) {
> rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-start",
> - binfo->loader_start + INITRD_LOAD_ADDR);
> + binfo->initrd_start);
> if (rc < 0) {
> fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
> }
>
> rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-end",
> - binfo->loader_start + INITRD_LOAD_ADDR +
> - binfo->initrd_size);
> + binfo->initrd_start + binfo->initrd_size);
> if (rc < 0) {
> fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
> }
> @@ -375,6 +374,19 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> big_endian = 0;
> #endif
>
> + /* We want to put the initrd far enough into RAM that when the
> + * kernel is uncompressed it will not clobber the initrd. However
> + * on boards without much RAM we must ensure that we still leave
> + * enough room for a decent sized initrd, and on boards with large
> + * amounts of RAM we must avoid the initrd being so far up in RAM
> + * that it is outside lowmem and inaccessible to the kernel.
> + * So for boards with less than 256MB of RAM we put the initrd
> + * halfway into RAM, and for boards with 256MB of RAM or more we put
> + * the initrd at 128MB.
> + */
> + info->initrd_start = info->loader_start +
> + MIN(info->ram_size / 2, 128 * 1024 * 1024);
> +
> /* Assume that raw images are linux kernels, and ELF images are not. */
> kernel_size = load_elf(info->kernel_filename, NULL, NULL, &elf_entry,
> NULL, NULL, big_endian, ELF_MACHINE, 1);
> @@ -398,10 +410,9 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> if (is_linux) {
> if (info->initrd_filename) {
> initrd_size = load_image_targphys(info->initrd_filename,
> - info->loader_start
> - + INITRD_LOAD_ADDR,
> - info->ram_size
> - - INITRD_LOAD_ADDR);
> + info->initrd_start,
> + info->ram_size -
> + info->initrd_start);
> if (initrd_size < 0) {
> fprintf(stderr, "qemu: could not load initrd '%s'\n",
> info->initrd_filename);
> @@ -419,9 +430,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> */
> if (info->dtb_filename) {
> /* Place the DTB after the initrd in memory */
> - hwaddr dtb_start = TARGET_PAGE_ALIGN(info->loader_start
> - + INITRD_LOAD_ADDR
> - + initrd_size);
> + hwaddr dtb_start = TARGET_PAGE_ALIGN(info->initrd_start +
> + initrd_size);
> if (load_dtb(dtb_start, info)) {
> exit(1);
> }
Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH v2] arm_boot: Change initrd load address to "halfway through RAM"
2012-10-26 15:29 [Qemu-devel] [PATCH v2] arm_boot: Change initrd load address to "halfway through RAM" Peter Maydell
2012-10-29 13:54 ` Aurelien Jarno
@ 2012-10-30 13:56 ` Igor Mitsyanko
2012-10-30 14:56 ` Cole Robinson
2012-11-01 16:05 ` Aurelien Jarno
3 siblings, 0 replies; 5+ messages in thread
From: Igor Mitsyanko @ 2012-10-30 13:56 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Crosthwaite, patches, Cole Robinson
On 10/26/2012 07:29 PM, Peter Maydell wrote:
> To avoid continually having to bump the initrd load address
> to account for larger kernel images, put the initrd halfway
> through RAM. This allows large kernels on new boards with lots
> of RAM to work OK, without breaking existing usecases for
> boards with only 32MB of RAM.
>
> Note that this change fixes in passing a bug where we were
> passing an overly large max_size to load_image_targphys()
> for the initrd, which meant that we wouldn't correctly refuse
> to load an enormous initrd that didn't actually fit into RAM.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Changes v1->v2: put initrd at min(128M, ram_size / 2)
> rather than just at ram_size / 2.
>
> hw/arm-misc.h | 1 +
> hw/arm_boot.c | 40 +++++++++++++++++++++++++---------------
> 2 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
> index d02f7f0..adb1665 100644
> --- a/hw/arm-misc.h
> +++ b/hw/arm-misc.h
> @@ -56,6 +56,7 @@ struct arm_boot_info {
> const struct arm_boot_info *info);
> /* Used internally by arm_boot.c */
> int is_linux;
> + hwaddr initrd_start;
> hwaddr initrd_size;
> hwaddr entry;
> };
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index 09bf6c5..92e2cab 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -18,7 +18,6 @@
>
> #define KERNEL_ARGS_ADDR 0x100
> #define KERNEL_LOAD_ADDR 0x00010000
> -#define INITRD_LOAD_ADDR 0x00d00000
>
> /* The worlds second smallest bootloader. Set r0-r2, then jump to kernel. */
> static uint32_t bootloader[] = {
> @@ -109,7 +108,7 @@ static void set_kernel_args(const struct arm_boot_info *info)
> /* ATAG_INITRD2 */
> WRITE_WORD(p, 4);
> WRITE_WORD(p, 0x54420005);
> - WRITE_WORD(p, info->loader_start + INITRD_LOAD_ADDR);
> + WRITE_WORD(p, info->initrd_start);
> WRITE_WORD(p, initrd_size);
> }
> if (info->kernel_cmdline && *info->kernel_cmdline) {
> @@ -185,10 +184,11 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
> /* pages_in_vram */
> WRITE_WORD(p, 0);
> /* initrd_start */
> - if (initrd_size)
> - WRITE_WORD(p, info->loader_start + INITRD_LOAD_ADDR);
> - else
> + if (initrd_size) {
> + WRITE_WORD(p, info->initrd_start);
> + } else {
> WRITE_WORD(p, 0);
> + }
> /* initrd_size */
> WRITE_WORD(p, initrd_size);
> /* rd_start */
> @@ -281,14 +281,13 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>
> if (binfo->initrd_size) {
> rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-start",
> - binfo->loader_start + INITRD_LOAD_ADDR);
> + binfo->initrd_start);
> if (rc < 0) {
> fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
> }
>
> rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-end",
> - binfo->loader_start + INITRD_LOAD_ADDR +
> - binfo->initrd_size);
> + binfo->initrd_start + binfo->initrd_size);
> if (rc < 0) {
> fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
> }
> @@ -375,6 +374,19 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> big_endian = 0;
> #endif
>
> + /* We want to put the initrd far enough into RAM that when the
> + * kernel is uncompressed it will not clobber the initrd. However
> + * on boards without much RAM we must ensure that we still leave
> + * enough room for a decent sized initrd, and on boards with large
> + * amounts of RAM we must avoid the initrd being so far up in RAM
> + * that it is outside lowmem and inaccessible to the kernel.
> + * So for boards with less than 256MB of RAM we put the initrd
> + * halfway into RAM, and for boards with 256MB of RAM or more we put
> + * the initrd at 128MB.
> + */
> + info->initrd_start = info->loader_start +
> + MIN(info->ram_size / 2, 128 * 1024 * 1024);
> +
> /* Assume that raw images are linux kernels, and ELF images are not. */
> kernel_size = load_elf(info->kernel_filename, NULL, NULL, &elf_entry,
> NULL, NULL, big_endian, ELF_MACHINE, 1);
> @@ -398,10 +410,9 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> if (is_linux) {
> if (info->initrd_filename) {
> initrd_size = load_image_targphys(info->initrd_filename,
> - info->loader_start
> - + INITRD_LOAD_ADDR,
> - info->ram_size
> - - INITRD_LOAD_ADDR);
> + info->initrd_start,
> + info->ram_size -
> + info->initrd_start);
> if (initrd_size < 0) {
> fprintf(stderr, "qemu: could not load initrd '%s'\n",
> info->initrd_filename);
> @@ -419,9 +430,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> */
> if (info->dtb_filename) {
> /* Place the DTB after the initrd in memory */
> - hwaddr dtb_start = TARGET_PAGE_ALIGN(info->loader_start
> - + INITRD_LOAD_ADDR
> - + initrd_size);
> + hwaddr dtb_start = TARGET_PAGE_ALIGN(info->initrd_start +
> + initrd_size);
> if (load_dtb(dtb_start, info)) {
> exit(1);
> }
>
Reviewed-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
--
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsyanko@samsung.com
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH v2] arm_boot: Change initrd load address to "halfway through RAM"
2012-10-26 15:29 [Qemu-devel] [PATCH v2] arm_boot: Change initrd load address to "halfway through RAM" Peter Maydell
2012-10-29 13:54 ` Aurelien Jarno
2012-10-30 13:56 ` Igor Mitsyanko
@ 2012-10-30 14:56 ` Cole Robinson
2012-11-01 16:05 ` Aurelien Jarno
3 siblings, 0 replies; 5+ messages in thread
From: Cole Robinson @ 2012-10-30 14:56 UTC (permalink / raw)
To: Peter Maydell; +Cc: Peter Crosthwaite, qemu-devel, patches
On 10/26/2012 11:29 AM, Peter Maydell wrote:
> To avoid continually having to bump the initrd load address
> to account for larger kernel images, put the initrd halfway
> through RAM. This allows large kernels on new boards with lots
> of RAM to work OK, without breaking existing usecases for
> boards with only 32MB of RAM.
>
> Note that this change fixes in passing a bug where we were
> passing an overly large max_size to load_image_targphys()
> for the initrd, which meant that we wouldn't correctly refuse
> to load an enormous initrd that didn't actually fit into RAM.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Changes v1->v2: put initrd at min(128M, ram_size / 2)
> rather than just at ram_size / 2.
I confirmed this patch fixes the original problem reported here:
https://bugzilla.redhat.com/show_bug.cgi?id=862766
Thanks Peter!
Tested-by: Cole Robinson <crobinso@redhat.com>
Thanks,
Cole
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] arm_boot: Change initrd load address to "halfway through RAM"
2012-10-26 15:29 [Qemu-devel] [PATCH v2] arm_boot: Change initrd load address to "halfway through RAM" Peter Maydell
` (2 preceding siblings ...)
2012-10-30 14:56 ` Cole Robinson
@ 2012-11-01 16:05 ` Aurelien Jarno
3 siblings, 0 replies; 5+ messages in thread
From: Aurelien Jarno @ 2012-11-01 16:05 UTC (permalink / raw)
To: Peter Maydell; +Cc: Peter Crosthwaite, patches, qemu-devel, Cole Robinson
On Fri, Oct 26, 2012 at 04:29:38PM +0100, Peter Maydell wrote:
> To avoid continually having to bump the initrd load address
> to account for larger kernel images, put the initrd halfway
> through RAM. This allows large kernels on new boards with lots
> of RAM to work OK, without breaking existing usecases for
> boards with only 32MB of RAM.
>
> Note that this change fixes in passing a bug where we were
> passing an overly large max_size to load_image_targphys()
> for the initrd, which meant that we wouldn't correctly refuse
> to load an enormous initrd that didn't actually fit into RAM.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Changes v1->v2: put initrd at min(128M, ram_size / 2)
> rather than just at ram_size / 2.
>
> hw/arm-misc.h | 1 +
> hw/arm_boot.c | 40 +++++++++++++++++++++++++---------------
> 2 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/hw/arm-misc.h b/hw/arm-misc.h
> index d02f7f0..adb1665 100644
> --- a/hw/arm-misc.h
> +++ b/hw/arm-misc.h
> @@ -56,6 +56,7 @@ struct arm_boot_info {
> const struct arm_boot_info *info);
> /* Used internally by arm_boot.c */
> int is_linux;
> + hwaddr initrd_start;
> hwaddr initrd_size;
> hwaddr entry;
> };
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index 09bf6c5..92e2cab 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -18,7 +18,6 @@
>
> #define KERNEL_ARGS_ADDR 0x100
> #define KERNEL_LOAD_ADDR 0x00010000
> -#define INITRD_LOAD_ADDR 0x00d00000
>
> /* The worlds second smallest bootloader. Set r0-r2, then jump to kernel. */
> static uint32_t bootloader[] = {
> @@ -109,7 +108,7 @@ static void set_kernel_args(const struct arm_boot_info *info)
> /* ATAG_INITRD2 */
> WRITE_WORD(p, 4);
> WRITE_WORD(p, 0x54420005);
> - WRITE_WORD(p, info->loader_start + INITRD_LOAD_ADDR);
> + WRITE_WORD(p, info->initrd_start);
> WRITE_WORD(p, initrd_size);
> }
> if (info->kernel_cmdline && *info->kernel_cmdline) {
> @@ -185,10 +184,11 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
> /* pages_in_vram */
> WRITE_WORD(p, 0);
> /* initrd_start */
> - if (initrd_size)
> - WRITE_WORD(p, info->loader_start + INITRD_LOAD_ADDR);
> - else
> + if (initrd_size) {
> + WRITE_WORD(p, info->initrd_start);
> + } else {
> WRITE_WORD(p, 0);
> + }
> /* initrd_size */
> WRITE_WORD(p, initrd_size);
> /* rd_start */
> @@ -281,14 +281,13 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>
> if (binfo->initrd_size) {
> rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-start",
> - binfo->loader_start + INITRD_LOAD_ADDR);
> + binfo->initrd_start);
> if (rc < 0) {
> fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
> }
>
> rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-end",
> - binfo->loader_start + INITRD_LOAD_ADDR +
> - binfo->initrd_size);
> + binfo->initrd_start + binfo->initrd_size);
> if (rc < 0) {
> fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
> }
> @@ -375,6 +374,19 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> big_endian = 0;
> #endif
>
> + /* We want to put the initrd far enough into RAM that when the
> + * kernel is uncompressed it will not clobber the initrd. However
> + * on boards without much RAM we must ensure that we still leave
> + * enough room for a decent sized initrd, and on boards with large
> + * amounts of RAM we must avoid the initrd being so far up in RAM
> + * that it is outside lowmem and inaccessible to the kernel.
> + * So for boards with less than 256MB of RAM we put the initrd
> + * halfway into RAM, and for boards with 256MB of RAM or more we put
> + * the initrd at 128MB.
> + */
> + info->initrd_start = info->loader_start +
> + MIN(info->ram_size / 2, 128 * 1024 * 1024);
> +
> /* Assume that raw images are linux kernels, and ELF images are not. */
> kernel_size = load_elf(info->kernel_filename, NULL, NULL, &elf_entry,
> NULL, NULL, big_endian, ELF_MACHINE, 1);
> @@ -398,10 +410,9 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> if (is_linux) {
> if (info->initrd_filename) {
> initrd_size = load_image_targphys(info->initrd_filename,
> - info->loader_start
> - + INITRD_LOAD_ADDR,
> - info->ram_size
> - - INITRD_LOAD_ADDR);
> + info->initrd_start,
> + info->ram_size -
> + info->initrd_start);
> if (initrd_size < 0) {
> fprintf(stderr, "qemu: could not load initrd '%s'\n",
> info->initrd_filename);
> @@ -419,9 +430,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
> */
> if (info->dtb_filename) {
> /* Place the DTB after the initrd in memory */
> - hwaddr dtb_start = TARGET_PAGE_ALIGN(info->loader_start
> - + INITRD_LOAD_ADDR
> - + initrd_size);
> + hwaddr dtb_start = TARGET_PAGE_ALIGN(info->initrd_start +
> + initrd_size);
> if (load_dtb(dtb_start, info)) {
> exit(1);
> }
Thanks, applied.
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 5+ messages in thread