| Message ID | 1418399932-7658-8-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12.12.14 16:58, Laszlo Ersek wrote: > Introduce the new boolean field "arm_boot_info.firmware_loaded". When this > field is set, it means that the portion of guest DRAM that the VCPU > normally starts to execute, or the pflash chip that the VCPU normally > starts to execute, has been populated by board-specific code with > full-fledged guest firmware code, before the board calls > arm_load_kernel(). > > Simultaneously, "arm_boot_info.firmware_loaded" guarantees that the board > code has set up the global firmware config instance, for arm_load_kernel() > to find with fw_cfg_find(). > > Guest kernel (-kernel) and guest firmware (-bios, -pflash) has always been > possible to specify independently on the command line. The following cases > should be considered: > > nr -bios -pflash -kernel description > unit#0 > -- ------- ------- ------- ------------------------------------------- > 1 present present absent Board code rejects this case, -bios and > present present present -pflash unit#0 are exclusive. Left intact > by this patch. > > 2 absent absent present Traditional kernel loading, with qemu's > minimal board firmware. Left intact by this > patch. > > 3 absent present absent Preexistent case for booting guest firmware > present absent absent loaded with -bios or -pflash. Left intact > by this patch. > > 4 absent absent absent Preexistent case for not loading any > firmware or kernel up-front. Left intact by > this patch. > > 5 present absent present New case introduced by this patch: kernel > absent present present image is passed to externally loaded > firmware in unmodified form, using fw_cfg. > > An easy way to see that this patch doesn't interfere with existing cases > is to realize that "info->firmware_loaded" is constant zero at this point. > Which makes the "outer" condition unchanged, and the "inner" condition > (with the fw_cfg-related code) dead. > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > --- > > Notes: > v4: > - drop space "between function name and open parenthesis '('" [Peter] > > v3: > - unchanged > > include/hw/arm/arm.h | 5 +++ > hw/arm/boot.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 91 insertions(+), 5 deletions(-) > > diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h > index cefc9e6..dd69d66 100644 > --- a/include/hw/arm/arm.h > +++ b/include/hw/arm/arm.h > @@ -65,8 +65,13 @@ struct arm_boot_info { > int is_linux; > hwaddr initrd_start; > hwaddr initrd_size; > hwaddr entry; > + > + /* Boot firmware has been loaded, typically at address 0, with -bios or > + * -pflash. It also implies that fw_cfg_find() will succeed. > + */ > + bool firmware_loaded; > }; > void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info); > > /* Multiplication factor to convert from system clock ticks to qemu timer > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > index e6a3c5b..5c65d16 100644 > --- a/hw/arm/boot.c > +++ b/hw/arm/boot.c > @@ -477,8 +477,62 @@ static void do_cpu_reset(void *opaque) > } > } > } > > +/** > + * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified > + * by key. > + * @fw_cfg: The firmware config instance to store the data in. > + * @size_key: The firmware config key to store the size of the loaded data > + * under, with fw_cfg_add_i32(). > + * @data_key: The firmware config key to store the loaded data under, with > + * fw_cfg_add_bytes(). > + * @image_name: The name of the image file to load. If it is NULL, the function > + * returns without doing anything. > + * @decompress: Whether the image should be decompressed (gunzipped) before > + * adding it to fw_cfg. > + * > + * In case of failure, the function prints an error message to stderr and the > + * process exits with status 1. > + */ > +static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key, > + uint16_t data_key, const char *image_name, > + bool decompress) > +{ > + size_t size; > + uint8_t *data; > + > + if (image_name == NULL) { > + return; > + } > + > + if (decompress) { > + int bytes; > + > + bytes = load_image_gzipped_buffer(image_name, > + LOAD_IMAGE_MAX_GUNZIP_BYTES, &data); > + if (bytes == -1) { > + fprintf(stderr, "failed to load and decompress \"%s\"\n", > + image_name); > + exit(1); > + } > + size = bytes; > + } else { > + gchar *contents; > + gsize length; > + > + if (!g_file_get_contents(image_name, &contents, &length, NULL)) { > + fprintf(stderr, "failed to load \"%s\"\n", image_name); > + exit(1); > + } > + size = length; > + data = (uint8_t *)contents; > + } > + > + fw_cfg_add_i32(fw_cfg, size_key, size); > + fw_cfg_add_bytes(fw_cfg, data_key, data, size); > +} > + > void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > { > CPUState *cs; > int kernel_size; > @@ -499,21 +553,48 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) > qemu_register_reset(do_cpu_reset, ARM_CPU(cs)); > } > > /* Load the kernel. */ > - if (!info->kernel_filename) { > + if (!info->kernel_filename || info->firmware_loaded) { > > if (have_dtb(info)) { > - /* If we have a device tree blob, but no kernel to supply it to, > - * copy it to the base of RAM for a bootloader to pick up. > + /* If we have a device tree blob, but no kernel to supply it to (or > + * the kernel is supposed to be loaded by the bootloader), copy the > + * DTB to the base of RAM for the bootloader to pick up. > */ > if (load_dtb(info->loader_start, info, 0) < 0) { > exit(1); > } > } > > - /* If no kernel specified, do nothing; we will start from address 0 > - * (typically a boot ROM image) in the same way as hardware. > + if (info->kernel_filename) { > + FWCfgState *fw_cfg; > + bool decompress_kernel; > + > + fw_cfg = fw_cfg_find(); > + decompress_kernel = arm_feature(&cpu->env, ARM_FEATURE_AARCH64); I don't understand this. On AArch64 I can simply do -kernel Image and it boots without decompressing anything, no? Alex
On 16 December 2014 at 12:15, Alexander Graf <agraf@suse.de> wrote: > I don't understand this. On AArch64 I can simply do -kernel Image and it > boots without decompressing anything, no? Yes, but if you pass -kernel Image.gz then it won't work unless the bootloader (ie QEMU) does the decompression for you. AArch64 differs from AArch32 here (on 32-bit compressed images have their own builtin decompressor; on 64-bit this is made the job of the boot loader). We could choose not to support loading compressed images, but that would be annoying for users (most other bootloaders do). See commit 6f5d3cbe88 where we added this support. -- PMM
On 16.12.14 13:18, Peter Maydell wrote: > On 16 December 2014 at 12:15, Alexander Graf <agraf@suse.de> wrote: >> I don't understand this. On AArch64 I can simply do -kernel Image and it >> boots without decompressing anything, no? > > Yes, but if you pass -kernel Image.gz then it won't work unless the > bootloader (ie QEMU) does the decompression for you. AArch64 differs > from AArch32 here (on 32-bit compressed images have their own builtin > decompressor; on 64-bit this is made the job of the boot loader). > We could choose not to support loading compressed images, but that > would be annoying for users (most other bootloaders do). See > commit 6f5d3cbe88 where we added this support. The patch as is assumes that AArch64 images are always gzipped. I don't think this assumption is correct - if you do "make Image" on a kernel source tree, you will get an uncompressed Image file. I think we'd be better off trying to load it as gzip and if it's not gzipped, fall back to linear load. Alex
On 16 December 2014 at 12:20, Alexander Graf <agraf@suse.de> wrote: > The patch as is assumes that AArch64 images are always gzipped. I don't > think this assumption is correct - if you do "make Image" on a kernel > source tree, you will get an uncompressed Image file. > > I think we'd be better off trying to load it as gzip and if it's not > gzipped, fall back to linear load. Ah, I see what you mean. Yes, we need to continue to support loading non-compressed Images as well as compressed ones, so this patch needs to do the try-and-fall-back, in the same way the current loader code does. thanks -- PMM
On Tue, Dec 16, 2014 at 12:25:41PM +0000, Peter Maydell wrote: > On 16 December 2014 at 12:20, Alexander Graf <agraf@suse.de> wrote: > > The patch as is assumes that AArch64 images are always gzipped. I don't > > think this assumption is correct - if you do "make Image" on a kernel > > source tree, you will get an uncompressed Image file. > > > > I think we'd be better off trying to load it as gzip and if it's not > > gzipped, fall back to linear load. > > Ah, I see what you mean. Yes, we need to continue to support > loading non-compressed Images as well as compressed ones, > so this patch needs to do the try-and-fall-back, in the > same way the current loader code does. Hang on, I tested the non-compressed case when I originally submitted the patch. And indeed it does work (still): (using qemu dfa9c2a0f4d0a0c8b2c1449ecdbb1297427e1560) $ cp /boot/vmlinuz-3.18.0-1.fc22.aarch64 /tmp/Image.gz $ gunzip /tmp/Image.gz $ ./aarch64-softmmu/qemu-system-aarch64 -kernel /tmp/Image -append 'console=ttyAMA0 earlyprintk=pl011,0x9000000 ignore_loglevel' No machine specified, and there is no default. Use -machine help to list supported machines! rjones@mustang:~/d/qemu$ ./aarch64-softmmu/qemu-system-aarch64 -M virt -cpu cortex-a57 -kernel /tmp/Image -append 'console=ttyAMA0 earlyprintk=pl011,0x9000000 ignore_loglevel' rjones@mustang:~/d/qemu$ rjones@mustang:~/d/qemu$ ./aarch64-softmmu/qemu-system-aarch64 -M virt -cpu cortex-a57 -kernel /tmp/Image -append 'console=ttyAMA0 earlyprintk=pl011,0x9000000 ignore_loglevel' -serial stdio [ 0.000000] Booting Linux on physical CPU 0x0 [ 0.000000] Initializing cgroup subsys cpuset [ 0.000000] Initializing cgroup subsys cpu [ 0.000000] Initializing cgroup subsys cpuacct [ 0.000000] Linux version 3.18.0-1.fc22.aarch64 (mockbuild@apm-mustang-ev3-11.ml3.eng.bos.redhat.com) (gcc version 4.9.2 20141101 (Red Hat 4.9.2-1) (GCC) ) #1 SMP Thu Dec 11 11:47:50 UTC 2014 [etc] And for completeness with a compressed kernel: $ ./aarch64-softmmu/qemu-system-aarch64 -M virt -cpu cortex-a57 -kernel /boot/vmlinuz-3.18.0-1.fc22.aarch64 -append 'console=ttyAMA0 earlyprintk=pl011,0x9000000 ignore_loglevel' -serial stdio [ 0.000000] Booting Linux on physical CPU 0x0 [ 0.000000] Initializing cgroup subsys cpuset [ 0.000000] Initializing cgroup subsys cpu [ 0.000000] Initializing cgroup subsys cpuacct [ 0.000000] Linux version 3.18.0-1.fc22.aarch64 (mockbuild@apm-mustang-ev3-11.ml3.eng.bos.redhat.com) (gcc version 4.9.2 20141101 (Red Hat 4.9.2-1) (GCC) ) #1 SMP Thu Dec 11 11:47:50 UTC 2014 [ 0.000000] CPU: AArch64 Processor [411fd070] revision 0 [etc] Rich.
On 12/16/14 13:42, Richard W.M. Jones wrote: > On Tue, Dec 16, 2014 at 12:25:41PM +0000, Peter Maydell wrote: >> On 16 December 2014 at 12:20, Alexander Graf <agraf@suse.de> wrote: >>> The patch as is assumes that AArch64 images are always gzipped. I don't >>> think this assumption is correct - if you do "make Image" on a kernel >>> source tree, you will get an uncompressed Image file. >>> >>> I think we'd be better off trying to load it as gzip and if it's not >>> gzipped, fall back to linear load. >> >> Ah, I see what you mean. Yes, we need to continue to support >> loading non-compressed Images as well as compressed ones, >> so this patch needs to do the try-and-fall-back, in the >> same way the current loader code does. > > Hang on, I tested the non-compressed case when I originally submitted > the patch. > > And indeed it does work (still): The bug is not in your patch; it's in mine, because it doesn't follow the fallback behavior that your patch correctly implements. I guess I'll get to use the get_image_size() / load_image_size() functions after all... Thanks Laszlo
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h index cefc9e6..dd69d66 100644 --- a/include/hw/arm/arm.h +++ b/include/hw/arm/arm.h @@ -65,8 +65,13 @@ struct arm_boot_info { int is_linux; hwaddr initrd_start; hwaddr initrd_size; hwaddr entry; + + /* Boot firmware has been loaded, typically at address 0, with -bios or + * -pflash. It also implies that fw_cfg_find() will succeed. + */ + bool firmware_loaded; }; void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info); /* Multiplication factor to convert from system clock ticks to qemu timer diff --git a/hw/arm/boot.c b/hw/arm/boot.c index e6a3c5b..5c65d16 100644 --- a/hw/arm/boot.c +++ b/hw/arm/boot.c @@ -477,8 +477,62 @@ static void do_cpu_reset(void *opaque) } } } +/** + * load_image_to_fw_cfg() - Load an image file into an fw_cfg entry identified + * by key. + * @fw_cfg: The firmware config instance to store the data in. + * @size_key: The firmware config key to store the size of the loaded data + * under, with fw_cfg_add_i32(). + * @data_key: The firmware config key to store the loaded data under, with + * fw_cfg_add_bytes(). + * @image_name: The name of the image file to load. If it is NULL, the function + * returns without doing anything. + * @decompress: Whether the image should be decompressed (gunzipped) before + * adding it to fw_cfg. + * + * In case of failure, the function prints an error message to stderr and the + * process exits with status 1. + */ +static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key, + uint16_t data_key, const char *image_name, + bool decompress) +{ + size_t size; + uint8_t *data; + + if (image_name == NULL) { + return; + } + + if (decompress) { + int bytes; + + bytes = load_image_gzipped_buffer(image_name, + LOAD_IMAGE_MAX_GUNZIP_BYTES, &data); + if (bytes == -1) { + fprintf(stderr, "failed to load and decompress \"%s\"\n", + image_name); + exit(1); + } + size = bytes; + } else { + gchar *contents; + gsize length; + + if (!g_file_get_contents(image_name, &contents, &length, NULL)) { + fprintf(stderr, "failed to load \"%s\"\n", image_name); + exit(1); + } + size = length; + data = (uint8_t *)contents; + } + + fw_cfg_add_i32(fw_cfg, size_key, size); + fw_cfg_add_bytes(fw_cfg, data_key, data, size); +} + void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) { CPUState *cs; int kernel_size; @@ -499,21 +553,48 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info) qemu_register_reset(do_cpu_reset, ARM_CPU(cs)); } /* Load the kernel. */ - if (!info->kernel_filename) { + if (!info->kernel_filename || info->firmware_loaded) { if (have_dtb(info)) { - /* If we have a device tree blob, but no kernel to supply it to, - * copy it to the base of RAM for a bootloader to pick up. + /* If we have a device tree blob, but no kernel to supply it to (or + * the kernel is supposed to be loaded by the bootloader), copy the + * DTB to the base of RAM for the bootloader to pick up. */ if (load_dtb(info->loader_start, info, 0) < 0) { exit(1); } } - /* If no kernel specified, do nothing; we will start from address 0 - * (typically a boot ROM image) in the same way as hardware. + if (info->kernel_filename) { + FWCfgState *fw_cfg; + bool decompress_kernel; + + fw_cfg = fw_cfg_find(); + decompress_kernel = arm_feature(&cpu->env, ARM_FEATURE_AARCH64); + + /* Expose the kernel, the command line, and the initrd in fw_cfg. + * We don't process them here at all, it's all left to the + * firmware. + */ + load_image_to_fw_cfg(fw_cfg, + FW_CFG_KERNEL_SIZE, FW_CFG_KERNEL_DATA, + info->kernel_filename, decompress_kernel); + load_image_to_fw_cfg(fw_cfg, + FW_CFG_INITRD_SIZE, FW_CFG_INITRD_DATA, + info->initrd_filename, false); + + if (info->kernel_cmdline) { + fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, + strlen(info->kernel_cmdline) + 1); + fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, + info->kernel_cmdline); + } + } + + /* We will start from address 0 (typically a boot ROM image) in the + * same way as hardware. */ return; }
Introduce the new boolean field "arm_boot_info.firmware_loaded". When this field is set, it means that the portion of guest DRAM that the VCPU normally starts to execute, or the pflash chip that the VCPU normally starts to execute, has been populated by board-specific code with full-fledged guest firmware code, before the board calls arm_load_kernel(). Simultaneously, "arm_boot_info.firmware_loaded" guarantees that the board code has set up the global firmware config instance, for arm_load_kernel() to find with fw_cfg_find(). Guest kernel (-kernel) and guest firmware (-bios, -pflash) has always been possible to specify independently on the command line. The following cases should be considered: nr -bios -pflash -kernel description unit#0 -- ------- ------- ------- ------------------------------------------- 1 present present absent Board code rejects this case, -bios and present present present -pflash unit#0 are exclusive. Left intact by this patch. 2 absent absent present Traditional kernel loading, with qemu's minimal board firmware. Left intact by this patch. 3 absent present absent Preexistent case for booting guest firmware present absent absent loaded with -bios or -pflash. Left intact by this patch. 4 absent absent absent Preexistent case for not loading any firmware or kernel up-front. Left intact by this patch. 5 present absent present New case introduced by this patch: kernel absent present present image is passed to externally loaded firmware in unmodified form, using fw_cfg. An easy way to see that this patch doesn't interfere with existing cases is to realize that "info->firmware_loaded" is constant zero at this point. Which makes the "outer" condition unchanged, and the "inner" condition (with the fw_cfg-related code) dead. Signed-off-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> --- Notes: v4: - drop space "between function name and open parenthesis '('" [Peter] v3: - unchanged include/hw/arm/arm.h | 5 +++ hw/arm/boot.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 91 insertions(+), 5 deletions(-)