Re: [Qemu-devel] [PATCH v4 7/8] hw/arm: pass pristine kernel image to guest firmware over fw_cfg

[Alexander Graf]

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 <address@hidden>
> Reviewed-by: Peter Maydell <address@hidden>
> ---
> 
> 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