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

[Laszlo Ersek]

On 12/12/14 14:20, Peter Maydell wrote:
> On 9 December 2014 at 01:13, Laszlo Ersek <address@hidden> 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>
>> ---
>>
>> Notes:
>>     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 0014c34..01d6d3d 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -475,8 +475,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);
> 
> Stray space again.

Will fix, thanks.

> 
>> +        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);
> 
> We have no way to free these buffers later but x86 does the same,
> so never mind.

OK.

> 
>> +}
>> +
>>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>  {
>>      CPUState *cs;
>>      int kernel_size;
>> @@ -497,21 +551,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);
> 
> If we have a real bootloader in the UEFI firmware, why do we need
> to do decompression for it? We only do this in the builtin bootloader
> because QEMU is acting as the bootloader and has to support the
> feature itself. I would have thought that the UEFI builtin kernel
> booting support already supported decompressing the kernel...

The "UEFI builtin kernel booting support" will qualify as "builtin" only
after my 2500 line patch series is merged in edk2.

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/11651

Zlib decompression is not present in edk2 (it only has a TianoCore
variant of LZMA), and I didn't want to impede (in the community sense)
my edk2 patchset even more (ie. beyond its current size) by importing
libz too.

Thanks
Laszlo

> 
> Otherwise looks OK.
> 
> -- PMM
>