hw/arm/boot.c | 8 +++++--- hw/core/loader.c | 19 ++++++++++++++++--- hw/core/uboot_image.h | 1 + hw/microblaze/boot.c | 2 +- hw/nios2/boot.c | 2 +- hw/ppc/e500.c | 1 + hw/ppc/ppc440_bamboo.c | 2 +- hw/ppc/sam460ex.c | 2 +- include/hw/loader.h | 7 ++++++- 9 files changed, 33 insertions(+), 11 deletions(-)
noload kernels are loaded with the u-boot image header and as a result
the header size needs adding to the entry point. Fake up a hdr so the
kernel image is loaded at the right address and the entry point is
adjusted appropriately.
The bootloader fits in the space that the uboot header would have occupied.
Clarify the load_uimage API to state the passing of a load address when an
image doesn't specify one, or when loading a ramdisk is expected.
Adjust callers of load_uimage, etc.
Signed-off-by: Nick Hudson<skrll@netbsd.org>
---
hw/arm/boot.c | 8 +++++---
hw/core/loader.c | 19 ++++++++++++++++---
hw/core/uboot_image.h | 1 +
hw/microblaze/boot.c | 2 +-
hw/nios2/boot.c | 2 +-
hw/ppc/e500.c | 1 +
hw/ppc/ppc440_bamboo.c | 2 +-
hw/ppc/sam460ex.c | 2 +-
include/hw/loader.h | 7 ++++++-
9 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 586baa9b64..450267566a 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -30,8 +30,9 @@
* Documentation/arm/Booting and Documentation/arm64/booting.txt
* They have different preferred image load offsets from system RAM base.
*/
-#define KERNEL_ARGS_ADDR 0x100
-#define KERNEL_LOAD_ADDR 0x00010000
+#define KERNEL_ARGS_ADDR 0x100
+#define KERNEL_NOLOAD_ADDR 0x00000000
+#define KERNEL_LOAD_ADDR 0x00010000
#define KERNEL64_LOAD_ADDR 0x00080000
#define ARM64_TEXT_OFFSET_OFFSET 8
@@ -1078,7 +1079,8 @@ void arm_load_kernel(ARMCPU *cpu, struct
arm_boot_info *info)
}
entry = elf_entry;
if (kernel_size < 0) {
- kernel_size = load_uimage_as(info->kernel_filename, &entry, NULL,
+ uint64_t loadaddr = info->loader_start + KERNEL_NOLOAD_ADDR;
+ kernel_size = load_uimage_as(info->kernel_filename, &entry,
&loadaddr,
&is_linux, NULL, NULL, as);
}
if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
diff --git a/hw/core/loader.c b/hw/core/loader.c
index aa0b3fc867..7362197162 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -638,13 +638,26 @@ static int load_uboot_image(const char *filename,
hwaddr *ep, hwaddr *loadaddr,
goto out;
if (hdr->ih_type != image_type) {
- fprintf(stderr, "Wrong image type %d, expected %d\n", hdr->ih_type,
- image_type);
- goto out;
+ if (image_type != IH_TYPE_KERNEL &&
+ hdr->ih_type != IH_TYPE_KERNEL_NOLOAD) {
+ fprintf(stderr, "Wrong image type %d, expected %d\n",
hdr->ih_type,
+ image_type);
+ goto out;
+ }
}
/* TODO: Implement other image types. */
switch (hdr->ih_type) {
+ case IH_TYPE_KERNEL_NOLOAD:
+ if (!loadaddr || *loadaddr == LOAD_UIMAGE_LOADADDR_INVALID) {
+ fprintf(stderr, "this image format (kernel_noload) cannot be "
+ "loaded on this machine type");
+ goto out;
+ }
+
+ hdr->ih_load = *loadaddr + sizeof(*hdr);
+ hdr->ih_ep += hdr->ih_load;
+ /* fall through */
case IH_TYPE_KERNEL:
address = hdr->ih_load;
if (translate_fn) {
diff --git a/hw/core/uboot_image.h b/hw/core/uboot_image.h
index 34c11a70a6..608022de6e 100644
--- a/hw/core/uboot_image.h
+++ b/hw/core/uboot_image.h
@@ -124,6 +124,7 @@
#define IH_TYPE_SCRIPT 6 /* Script file */
#define IH_TYPE_FILESYSTEM 7 /* Filesystem Image (any type) */
#define IH_TYPE_FLATDT 8 /* Binary Flat Device Tree Blob */
+#define IH_TYPE_KERNEL_NOLOAD 14 /* OS Kernel Image (noload) */
/*
* Compression Types
diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 35bfeda7aa..489ab839b7 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -156,7 +156,7 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu,
hwaddr ddr_base,
/* If it wasn't an ELF image, try an u-boot image. */
if (kernel_size < 0) {
- hwaddr uentry, loadaddr;
+ hwaddr uentry, loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
kernel_size = load_uimage(kernel_filename, &uentry,
&loadaddr, 0,
NULL, NULL);
diff --git a/hw/nios2/boot.c b/hw/nios2/boot.c
index 4bb5b601d3..ed5cb28e94 100644
--- a/hw/nios2/boot.c
+++ b/hw/nios2/boot.c
@@ -161,7 +161,7 @@ void nios2_load_kernel(Nios2CPU *cpu, hwaddr ddr_base,
/* If it wasn't an ELF image, try an u-boot image. */
if (kernel_size < 0) {
- hwaddr uentry, loadaddr;
+ hwaddr uentry, loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
kernel_size = load_uimage(kernel_filename, &uentry,
&loadaddr, 0,
NULL, NULL);
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index e6747fce28..e275178951 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -997,6 +997,7 @@ void ppce500_init(MachineState *machine)
* Hrm. No ELF image? Try a uImage, maybe someone is giving us an
* ePAPR compliant kernel
*/
+ loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
payload_size = load_uimage(filename, &bios_entry, &loadaddr, NULL,
NULL, NULL);
if (payload_size < 0) {
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index f5720f979e..70428b1865 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -180,7 +180,7 @@ static void bamboo_init(MachineState *machine)
CPUPPCState *env;
uint64_t elf_entry;
uint64_t elf_lowaddr;
- hwaddr loadaddr = 0;
+ hwaddr loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
target_long initrd_size = 0;
DeviceState *dev;
int success;
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 5aac58f36e..2d6d5c4402 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -402,7 +402,7 @@ static void sam460ex_init(MachineState *machine)
CPUPPCState *env;
PPC4xxI2CState *i2c[2];
hwaddr entry = UBOOT_ENTRY;
- hwaddr loadaddr = 0;
+ hwaddr loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
target_long initrd_size = 0;
DeviceState *dev;
SysBusDevice *sbdev;
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 67a0af84ac..a7254bc5c4 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -160,10 +160,15 @@ void load_elf_hdr(const char *filename, void *hdr,
bool *is64, Error **errp);
int load_aout(const char *filename, hwaddr addr, int max_sz,
int bswap_needed, hwaddr target_page_size);
+#define LOAD_UIMAGE_LOADADDR_INVALID (-1)
+
/** load_uimage_as:
* @filename: Path of uimage file
* @ep: Populated with program entry point. Ignored if NULL.
- * @loadaddr: Populated with the load address. Ignored if NULL.
+ * @loadaddr: load address if none specified in the image or when
loading a ramdisk.
+ * Populated with the the load address. Ignored if NULL or
+ * LOAD_UIMAGE_LOADADDR_INVALID (images which do not specify
a load address
+ * will not be loadable).
* @is_linux: Is set to true if the image loaded is Linux. Ignored if
NULL.
* @translate_fn: optional function to translate load addresses
* @translate_opaque: opaque data passed to @translate_fn
--
2.17.1
ping
On 11/12/2018 12:27, Nick Hudson wrote:
>
> noload kernels are loaded with the u-boot image header and as a result
> the header size needs adding to the entry point. Fake up a hdr so the
> kernel image is loaded at the right address and the entry point is
> adjusted appropriately.
>
> The bootloader fits in the space that the uboot header would have
> occupied.
>
> Clarify the load_uimage API to state the passing of a load address
> when an
> image doesn't specify one, or when loading a ramdisk is expected.
>
> Adjust callers of load_uimage, etc.
>
> Signed-off-by: Nick Hudson<skrll@netbsd.org>
>
> ---
> hw/arm/boot.c | 8 +++++---
> hw/core/loader.c | 19 ++++++++++++++++---
> hw/core/uboot_image.h | 1 +
> hw/microblaze/boot.c | 2 +-
> hw/nios2/boot.c | 2 +-
> hw/ppc/e500.c | 1 +
> hw/ppc/ppc440_bamboo.c | 2 +-
> hw/ppc/sam460ex.c | 2 +-
> include/hw/loader.h | 7 ++++++-
> 9 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 586baa9b64..450267566a 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -30,8 +30,9 @@
> * Documentation/arm/Booting and Documentation/arm64/booting.txt
> * They have different preferred image load offsets from system RAM
> base.
> */
> -#define KERNEL_ARGS_ADDR 0x100
> -#define KERNEL_LOAD_ADDR 0x00010000
> +#define KERNEL_ARGS_ADDR 0x100
> +#define KERNEL_NOLOAD_ADDR 0x00000000
> +#define KERNEL_LOAD_ADDR 0x00010000
> #define KERNEL64_LOAD_ADDR 0x00080000
>
> #define ARM64_TEXT_OFFSET_OFFSET 8
> @@ -1078,7 +1079,8 @@ void arm_load_kernel(ARMCPU *cpu, struct
> arm_boot_info *info)
> }
> entry = elf_entry;
> if (kernel_size < 0) {
> - kernel_size = load_uimage_as(info->kernel_filename, &entry,
> NULL,
> + uint64_t loadaddr = info->loader_start + KERNEL_NOLOAD_ADDR;
> + kernel_size = load_uimage_as(info->kernel_filename, &entry,
> &loadaddr,
> &is_linux, NULL, NULL, as);
> }
> if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size <
> 0) {
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index aa0b3fc867..7362197162 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -638,13 +638,26 @@ static int load_uboot_image(const char
> *filename, hwaddr *ep, hwaddr *loadaddr,
> goto out;
>
> if (hdr->ih_type != image_type) {
> - fprintf(stderr, "Wrong image type %d, expected %d\n",
> hdr->ih_type,
> - image_type);
> - goto out;
> + if (image_type != IH_TYPE_KERNEL &&
> + hdr->ih_type != IH_TYPE_KERNEL_NOLOAD) {
> + fprintf(stderr, "Wrong image type %d, expected %d\n",
> hdr->ih_type,
> + image_type);
> + goto out;
> + }
> }
>
> /* TODO: Implement other image types. */
> switch (hdr->ih_type) {
> + case IH_TYPE_KERNEL_NOLOAD:
> + if (!loadaddr || *loadaddr == LOAD_UIMAGE_LOADADDR_INVALID) {
> + fprintf(stderr, "this image format (kernel_noload) cannot
> be "
> + "loaded on this machine type");
> + goto out;
> + }
> +
> + hdr->ih_load = *loadaddr + sizeof(*hdr);
> + hdr->ih_ep += hdr->ih_load;
> + /* fall through */
> case IH_TYPE_KERNEL:
> address = hdr->ih_load;
> if (translate_fn) {
> diff --git a/hw/core/uboot_image.h b/hw/core/uboot_image.h
> index 34c11a70a6..608022de6e 100644
> --- a/hw/core/uboot_image.h
> +++ b/hw/core/uboot_image.h
> @@ -124,6 +124,7 @@
> #define IH_TYPE_SCRIPT 6 /* Script file */
> #define IH_TYPE_FILESYSTEM 7 /* Filesystem Image (any type) */
> #define IH_TYPE_FLATDT 8 /* Binary Flat Device Tree Blob */
> +#define IH_TYPE_KERNEL_NOLOAD 14 /* OS Kernel Image (noload) */
>
> /*
> * Compression Types
> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
> index 35bfeda7aa..489ab839b7 100644
> --- a/hw/microblaze/boot.c
> +++ b/hw/microblaze/boot.c
> @@ -156,7 +156,7 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu,
> hwaddr ddr_base,
>
> /* If it wasn't an ELF image, try an u-boot image. */
> if (kernel_size < 0) {
> - hwaddr uentry, loadaddr;
> + hwaddr uentry, loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
>
> kernel_size = load_uimage(kernel_filename, &uentry,
> &loadaddr, 0,
> NULL, NULL);
> diff --git a/hw/nios2/boot.c b/hw/nios2/boot.c
> index 4bb5b601d3..ed5cb28e94 100644
> --- a/hw/nios2/boot.c
> +++ b/hw/nios2/boot.c
> @@ -161,7 +161,7 @@ void nios2_load_kernel(Nios2CPU *cpu, hwaddr
> ddr_base,
>
> /* If it wasn't an ELF image, try an u-boot image. */
> if (kernel_size < 0) {
> - hwaddr uentry, loadaddr;
> + hwaddr uentry, loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
>
> kernel_size = load_uimage(kernel_filename, &uentry,
> &loadaddr, 0,
> NULL, NULL);
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index e6747fce28..e275178951 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -997,6 +997,7 @@ void ppce500_init(MachineState *machine)
> * Hrm. No ELF image? Try a uImage, maybe someone is giving
> us an
> * ePAPR compliant kernel
> */
> + loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
> payload_size = load_uimage(filename, &bios_entry, &loadaddr,
> NULL,
> NULL, NULL);
> if (payload_size < 0) {
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index f5720f979e..70428b1865 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -180,7 +180,7 @@ static void bamboo_init(MachineState *machine)
> CPUPPCState *env;
> uint64_t elf_entry;
> uint64_t elf_lowaddr;
> - hwaddr loadaddr = 0;
> + hwaddr loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
> target_long initrd_size = 0;
> DeviceState *dev;
> int success;
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 5aac58f36e..2d6d5c4402 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -402,7 +402,7 @@ static void sam460ex_init(MachineState *machine)
> CPUPPCState *env;
> PPC4xxI2CState *i2c[2];
> hwaddr entry = UBOOT_ENTRY;
> - hwaddr loadaddr = 0;
> + hwaddr loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
> target_long initrd_size = 0;
> DeviceState *dev;
> SysBusDevice *sbdev;
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 67a0af84ac..a7254bc5c4 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -160,10 +160,15 @@ void load_elf_hdr(const char *filename, void
> *hdr, bool *is64, Error **errp);
> int load_aout(const char *filename, hwaddr addr, int max_sz,
> int bswap_needed, hwaddr target_page_size);
>
> +#define LOAD_UIMAGE_LOADADDR_INVALID (-1)
> +
> /** load_uimage_as:
> * @filename: Path of uimage file
> * @ep: Populated with program entry point. Ignored if NULL.
> - * @loadaddr: Populated with the load address. Ignored if NULL.
> + * @loadaddr: load address if none specified in the image or when
> loading a ramdisk.
> + * Populated with the the load address. Ignored if NULL or
> + * LOAD_UIMAGE_LOADADDR_INVALID (images which do not
> specify a load address
> + * will not be loadable).
> * @is_linux: Is set to true if the image loaded is Linux. Ignored if
> NULL.
> * @translate_fn: optional function to translate load addresses
> * @translate_opaque: opaque data passed to @translate_fn
On Fri, 21 Dec 2018 at 16:13, Nick Hudson <nicholas.a.hudson@gmail.com> wrote: > > ping Thanks for the ping. This is on my to-review list but I'm not going to be doing any code review until after I get back to work in January now... thanks -- PMM
On Tue, 11 Dec 2018 at 12:27, Nick Hudson <nicholas.a.hudson@gmail.com> wrote:
>
>
> noload kernels are loaded with the u-boot image header and as a result
> the header size needs adding to the entry point. Fake up a hdr so the
> kernel image is loaded at the right address and the entry point is
> adjusted appropriately.
>
> The bootloader fits in the space that the uboot header would have occupied.
>
> Clarify the load_uimage API to state the passing of a load address when an
> image doesn't specify one, or when loading a ramdisk is expected.
>
> Adjust callers of load_uimage, etc.
>
> Signed-off-by: Nick Hudson<skrll@netbsd.org>
>
> ---
> hw/arm/boot.c | 8 +++++---
> hw/core/loader.c | 19 ++++++++++++++++---
> hw/core/uboot_image.h | 1 +
> hw/microblaze/boot.c | 2 +-
> hw/nios2/boot.c | 2 +-
> hw/ppc/e500.c | 1 +
> hw/ppc/ppc440_bamboo.c | 2 +-
> hw/ppc/sam460ex.c | 2 +-
> include/hw/loader.h | 7 ++++++-
> 9 files changed, 33 insertions(+), 11 deletions(-)
Hi; I tried applying your patch, but unfortunately it looks like
your email client has mangled it to the point that it can't
be applied:
* it is format=flowed, so long lines have been wrapped
* some whitespace has been turned into UTF-8 non-breaking space characters
I tried to manually fix these up, but didn't succeed.
It looks like you're using Thunderbird -- the kernel docs
have some config setting suggestions that might help:
https://www.kernel.org/doc/html/v4.11/process/email-clients.html#thunderbird-gui
or you could try switching to 'git send-email'.
I've reviewed the patch for content and it's mostly good:
I just have a couple more questions below.
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 586baa9b64..450267566a 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -30,8 +30,9 @@
> * Documentation/arm/Booting and Documentation/arm64/booting.txt
> * They have different preferred image load offsets from system RAM base.
> */
> -#define KERNEL_ARGS_ADDR 0x100
> -#define KERNEL_LOAD_ADDR 0x00010000
> +#define KERNEL_ARGS_ADDR 0x100
> +#define KERNEL_NOLOAD_ADDR 0x00000000
If I'm reading the code right, this requests that noload images
are loaded at offset zero into RAM, yes ? That's not a great
choice, because we put our little mini bootloader at offset 0,
and so the two will clash. Can we put them somewhere else
(above BOOTLOADER_MAX_SIZE, probably at a 64K page boundary) ?
If they must go at offset 0 then we'd need to refuse to load
noload images if they set is_linux to true. (We only load the
mini bootloader if is_linux is true, so that's the only case where
there's a clash.)
> +#define KERNEL_LOAD_ADDR 0x00010000
> #define KERNEL64_LOAD_ADDR 0x00080000
>
> #define ARM64_TEXT_OFFSET_OFFSET 8
> @@ -1078,7 +1079,8 @@ void arm_load_kernel(ARMCPU *cpu, struct
> arm_boot_info *info)
> }
> entry = elf_entry;
> if (kernel_size < 0) {
> - kernel_size = load_uimage_as(info->kernel_filename, &entry, NULL,
> + uint64_t loadaddr = info->loader_start + KERNEL_NOLOAD_ADDR;
> + kernel_size = load_uimage_as(info->kernel_filename, &entry,
> &loadaddr,
> &is_linux, NULL, NULL, as);
> }
> if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index aa0b3fc867..7362197162 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -638,13 +638,26 @@ static int load_uboot_image(const char *filename,
> hwaddr *ep, hwaddr *loadaddr,
> goto out;
>
> if (hdr->ih_type != image_type) {
> - fprintf(stderr, "Wrong image type %d, expected %d\n", hdr->ih_type,
> - image_type);
> - goto out;
> + if (image_type != IH_TYPE_KERNEL &&
> + hdr->ih_type != IH_TYPE_KERNEL_NOLOAD) {
I think you have this condition wrong. Suppose we are
trying to load image_type == IH_TYPE_KERNEL and we are
passed a hdr->ih_type == IH_TYPE_RAMDISK. With this change
we'll stop rejecting that as an error.
I think you want:
if (!(image_type == IH_TYPE_KERNEL &&
hdr->ih_type == IH_TYPE_KERNEL_NOLOAD)) {
because the only kind of mismatch we want to accept is
"we're looking for KERNEL and we got KERNEL_NOLOAD".
> + fprintf(stderr, "Wrong image type %d, expected %d\n",
> hdr->ih_type,
> + image_type);
> + goto out;
> + }
> }
> index f5720f979e..70428b1865 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -180,7 +180,7 @@ static void bamboo_init(MachineState *machine)
> CPUPPCState *env;
> uint64_t elf_entry;
> uint64_t elf_lowaddr;
> - hwaddr loadaddr = 0;
> + hwaddr loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
> target_long initrd_size = 0;
> DeviceState *dev;
> int success;
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 5aac58f36e..2d6d5c4402 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -402,7 +402,7 @@ static void sam460ex_init(MachineState *machine)
> CPUPPCState *env;
> PPC4xxI2CState *i2c[2];
> hwaddr entry = UBOOT_ENTRY;
> - hwaddr loadaddr = 0;
> + hwaddr loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
> target_long initrd_size = 0;
> DeviceState *dev;
> SysBusDevice *sbdev;
In both of these cases we could in fact remove the 'loadaddr'
variable entirely (and pass NULL to the load_uimage() 3rd
argument) because the code doesn't ever read the loadaddr value
that is set. But this is the simple minimal change so it's OK:
we can do the cleanup later (or not at all).
thanks
-- PMM
On 03/01/2019 16:20, Peter Maydell wrote:
> On Tue, 11 Dec 2018 at 12:27, Nick Hudson <nicholas.a.hudson@gmail.com> wrote:
>>
>>
>> noload kernels are loaded with the u-boot image header and as a result
>> the header size needs adding to the entry point. Fake up a hdr so the
>> kernel image is loaded at the right address and the entry point is
>> adjusted appropriately.
>>
>> The bootloader fits in the space that the uboot header would have occupied.
>>
>> Clarify the load_uimage API to state the passing of a load address when an
>> image doesn't specify one, or when loading a ramdisk is expected.
>>
>> Adjust callers of load_uimage, etc.
>>
>> Signed-off-by: Nick Hudson<skrll@netbsd.org>
>>
>> ---
>> hw/arm/boot.c | 8 +++++---
>> hw/core/loader.c | 19 ++++++++++++++++---
>> hw/core/uboot_image.h | 1 +
>> hw/microblaze/boot.c | 2 +-
>> hw/nios2/boot.c | 2 +-
>> hw/ppc/e500.c | 1 +
>> hw/ppc/ppc440_bamboo.c | 2 +-
>> hw/ppc/sam460ex.c | 2 +-
>> include/hw/loader.h | 7 ++++++-
>> 9 files changed, 33 insertions(+), 11 deletions(-)
>
> Hi; I tried applying your patch, but unfortunately it looks like
> your email client has mangled it to the point that it can't
> be applied:
> * it is format=flowed, so long lines have been wrapped
> * some whitespace has been turned into UTF-8 non-breaking space characters
> I tried to manually fix these up, but didn't succeed.
>
> It looks like you're using Thunderbird -- the kernel docs
> have some config setting suggestions that might help:
> https://www.kernel.org/doc/html/v4.11/process/email-clients.html#thunderbird-gui
> or you could try switching to 'git send-email'.
>
> I've reviewed the patch for content and it's mostly good:
> I just have a couple more questions below.
>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 586baa9b64..450267566a 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -30,8 +30,9 @@
>> * Documentation/arm/Booting and Documentation/arm64/booting.txt
>> * They have different preferred image load offsets from system RAM base.
>> */
>> -#define KERNEL_ARGS_ADDR 0x100
>> -#define KERNEL_LOAD_ADDR 0x00010000
>> +#define KERNEL_ARGS_ADDR 0x100
>> +#define KERNEL_NOLOAD_ADDR 0x00000000
>
> If I'm reading the code right, this requests that noload images
> are loaded at offset zero into RAM, yes ?
Not quite.
They're loaded as if the full noload image (including uboot header)
was loaded at offset zero, but as load_uboot_image doesn't load the
header then the image body (minus the header) is loaded at offset
zero + the size of the u-boot header, i.e. 0x40.
> That's not a great
> choice, because we put our little mini bootloader at offset 0,
> and so the two will clash.
Fortuntately, the bootloader fits in the space that the uboot header would
have occupied.
My commit message has
"The bootloader fits in the space that the uboot header would have occupied."
to try and described this. I could add more of a description if required?
> Can we put them somewhere else
> (above BOOTLOADER_MAX_SIZE, probably at a 64K page boundary) ?
I don't think this is necessary.
> If they must go at offset 0 then we'd need to refuse to load
> noload images if they set is_linux to true. (We only load the
> mini bootloader if is_linux is true, so that's the only case where
> there's a clash.)
Nor this.
>
>> +#define KERNEL_LOAD_ADDR 0x00010000
>> #define KERNEL64_LOAD_ADDR 0x00080000
>>
>> #define ARM64_TEXT_OFFSET_OFFSET 8
>> @@ -1078,7 +1079,8 @@ void arm_load_kernel(ARMCPU *cpu, struct
>> arm_boot_info *info)
>> }
>> entry = elf_entry;
>> if (kernel_size < 0) {
>> - kernel_size = load_uimage_as(info->kernel_filename, &entry, NULL,
>> + uint64_t loadaddr = info->loader_start + KERNEL_NOLOAD_ADDR;
>> + kernel_size = load_uimage_as(info->kernel_filename, &entry,
>> &loadaddr,
>> &is_linux, NULL, NULL, as);
>> }
>> if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index aa0b3fc867..7362197162 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -638,13 +638,26 @@ static int load_uboot_image(const char *filename,
>> hwaddr *ep, hwaddr *loadaddr,
>> goto out;
>>
>> if (hdr->ih_type != image_type) {
>> - fprintf(stderr, "Wrong image type %d, expected %d\n", hdr->ih_type,
>> - image_type);
>> - goto out;
>> + if (image_type != IH_TYPE_KERNEL &&
>> + hdr->ih_type != IH_TYPE_KERNEL_NOLOAD) {
>
> I think you have this condition wrong. Suppose we are
> trying to load image_type == IH_TYPE_KERNEL and we are
> passed a hdr->ih_type == IH_TYPE_RAMDISK. With this change
> we'll stop rejecting that as an error.
> I think you want:
> if (!(image_type == IH_TYPE_KERNEL &&
> hdr->ih_type == IH_TYPE_KERNEL_NOLOAD)) {
>
> because the only kind of mismatch we want to accept is
> "we're looking for KERNEL and we got KERNEL_NOLOAD".
yeah, thanks.
[snip]
>
> thanks
> -- PMM
>
thanks for the review.
I'll send v4 with only the condition fixed and more descritive commit
message (and correct formatting). ok?
Nick
On Thu, 3 Jan 2019 at 16:50, Nick Hudson <nicholas.a.hudson@gmail.com> wrote: > On 03/01/2019 16:20, Peter Maydell wrote: > > On Tue, 11 Dec 2018 at 12:27, Nick Hudson <nicholas.a.hudson@gmail.com> wrote: > >> --- a/hw/arm/boot.c > >> +++ b/hw/arm/boot.c > >> @@ -30,8 +30,9 @@ > >> * Documentation/arm/Booting and Documentation/arm64/booting.txt > >> * They have different preferred image load offsets from system RAM base. > >> */ > >> -#define KERNEL_ARGS_ADDR 0x100 > >> -#define KERNEL_LOAD_ADDR 0x00010000 > >> +#define KERNEL_ARGS_ADDR 0x100 > >> +#define KERNEL_NOLOAD_ADDR 0x00000000 > > > > If I'm reading the code right, this requests that noload images > > are loaded at offset zero into RAM, yes ? > > Not quite. > > They're loaded as if the full noload image (including uboot header) > was loaded at offset zero, but as load_uboot_image doesn't load the > header then the image body (minus the header) is loaded at offset > zero + the size of the u-boot header, i.e. 0x40. > > > That's not a great > > choice, because we put our little mini bootloader at offset 0, > > and so the two will clash. > > Fortuntately, the bootloader fits in the space that the uboot header would > have occupied. > > My commit message has > > "The bootloader fits in the space that the uboot header would have occupied." > > to try and described this. I could add more of a description if required? Ah, I missed that. This seems very fragile to me -- 0x40 is only 64 bytes, which is 16 instructions. Currently our boot loader code is less than that, but only by four insns or so, so it's very plausible that some future enhancement to the loader code would take it over the 0x40 boundary, at which point handling of noload images will silently fail. At a minimum we should have an assertion that we stay below 0x40; but I'm not keen on restricting ourselves to 16-instruction bootloaders. Will a noload image ever in practice also be a Linux kernel (ie hdr->ih_os == IH_OS_LINUX) ? If not, then we don't lose anything by not allowing those to be loaded. (And if anybody does in future complain that their image doesn't work, we could make it supported by allowing the mini-bootloader to be placed elsewhere in RAM.) thanks -- PMM
On 03/01/2019 17:27, Peter Maydell wrote: > On Thu, 3 Jan 2019 at 16:50, Nick Hudson <nicholas.a.hudson@gmail.com> wrote: >> On 03/01/2019 16:20, Peter Maydell wrote: >>> On Tue, 11 Dec 2018 at 12:27, Nick Hudson <nicholas.a.hudson@gmail.com> wrote: >>>> --- a/hw/arm/boot.c >>>> +++ b/hw/arm/boot.c >>>> @@ -30,8 +30,9 @@ >>>> * Documentation/arm/Booting and Documentation/arm64/booting.txt >>>> * They have different preferred image load offsets from system RAM base. >>>> */ >>>> -#define KERNEL_ARGS_ADDR 0x100 >>>> -#define KERNEL_LOAD_ADDR 0x00010000 >>>> +#define KERNEL_ARGS_ADDR 0x100 >>>> +#define KERNEL_NOLOAD_ADDR 0x00000000 >>> >>> If I'm reading the code right, this requests that noload images >>> are loaded at offset zero into RAM, yes ? >> >> Not quite. >> >> They're loaded as if the full noload image (including uboot header) >> was loaded at offset zero, but as load_uboot_image doesn't load the >> header then the image body (minus the header) is loaded at offset >> zero + the size of the u-boot header, i.e. 0x40. >> >>> That's not a great >>> choice, because we put our little mini bootloader at offset 0, >>> and so the two will clash. >> >> Fortuntately, the bootloader fits in the space that the uboot header would >> have occupied. >> >> My commit message has >> >> "The bootloader fits in the space that the uboot header would have occupied." >> >> to try and described this. I could add more of a description if required? > > Ah, I missed that. This seems very fragile to me -- 0x40 > is only 64 bytes, which is 16 instructions. Currently > our boot loader code is less than that, but only by four > insns or so, so it's very plausible that some future > enhancement to the loader code would take it over the 0x40 > boundary, at which point handling of noload images will > silently fail. At a minimum we should have an assertion > that we stay below 0x40; but I'm not keen on restricting > ourselves to 16-instruction bootloaders. I can add an assertion. Kernels often like to be 2MiB aligned to allow for more simple translation tables. This and the fact that the bootloader hasn't changed in 3 years I'd like to kick this can down the road. > > Will a noload image ever in practice also be a Linux kernel > (ie hdr->ih_os == IH_OS_LINUX) ? NetBSD/FreeBSD/OpenBSD (ab)use IH_OS_LINUX. They and Linux > If not, then we don't lose > anything by not allowing those to be loaded. (And if anybody > does in future complain that their image doesn't work, we > could make it supported by allowing the mini-bootloader to > be placed elsewhere in RAM.) > > thanks > -- PMM > thanks, Nick
© 2016 - 2024 Red Hat, Inc.