Hi,
On 14/12/2018 18:08, Andre Przywara wrote:
> On Tue, 4 Dec 2018 11:14:31 +0000
> Julien Thierry <julien.thierry@xxxxxxx> wrote:
>
> Hi,
>
>> Implement firmware image loading for arm and set the boot start
>> address to the firmware address.
>>
>> Add an option for the user to specify where to map the firmware.
>
> How is the user supposed to know this address? Will EDKII be linked
> against a certain address, which has to be reflected in this parameter?
>
For EDKII I believe any address in RAM is fine (although I'm unsure
whether there are other alignment properties).
For other firmwares/bootloaders, only the user can know depending on
what they are loading.
> Wouldn't it be feasible to provide a default value, say the beginning
> of DRAM? (See below)
>
>> Signed-off-by: Julien Thierry <julien.thierry@xxxxxxx>
>> ---
>> arm/fdt.c | 14 +++++++-
>> arm/include/arm-common/kvm-config-arch.h | 5 ++-
>> arm/kvm.c | 58
>> +++++++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 3
>> deletions(-)
>>
>> diff --git a/arm/fdt.c b/arm/fdt.c
>> index 664bb62..2936986 100644
>> --- a/arm/fdt.c
>> +++ b/arm/fdt.c
>> @@ -131,7 +131,19 @@ static int setup_fdt(struct kvm *kvm)
>> /* /chosen */
>> _FDT(fdt_begin_node(fdt, "chosen"));
>> _FDT(fdt_property_cell(fdt, "linux,pci-probe-only", 1));
>> - _FDT(fdt_property_string(fdt, "bootargs",
>> kvm->cfg.real_cmdline)); +
>> + if (kvm->cfg.firmware_filename) {
>> + /*
>> + * When using a firmware, command line is not passed
>> through DT,
>> + * or the firmware can add it itself
>> + */
>> + if (kvm->cfg.kernel_cmdline)
>> + pr_warning("Ignoring custom bootargs: %s\n",
>> + kvm->cfg.kernel_cmdline);
>> + } else
>> + _FDT(fdt_property_string(fdt, "bootargs",
>> + kvm->cfg.real_cmdline));
>> +
>> _FDT(fdt_property_u64(fdt, "kaslr-seed",
>> kvm->cfg.arch.kaslr_seed));
>> /* Initrd */
>> diff --git a/arm/include/arm-common/kvm-config-arch.h
>> b/arm/include/arm-common/kvm-config-arch.h index 6a196f1..5734c46
>> 100644 --- a/arm/include/arm-common/kvm-config-arch.h
>> +++ b/arm/include/arm-common/kvm-config-arch.h
>> @@ -11,6 +11,7 @@ struct kvm_config_arch {
>> bool has_pmuv3;
>> u64 kaslr_seed;
>> enum irqchip_type irqchip;
>> + u64 fw_addr;
>> };
>>
>> int irqchip_parser(const struct option *opt, const char *arg, int
>> unset); @@ -30,6 +31,8 @@ int irqchip_parser(const struct option
>> *opt, const char *arg, int unset); OPT_CALLBACK('\0', "irqchip",
>> &(cfg)->irqchip, \
>> "[gicv2|gicv2m|gicv3|gicv3-its]", \
>> "Type of interrupt controller to emulate in the guest", \
>> - irqchip_parser, NULL),
>> + irqchip_parser,
>> NULL), \
>> + OPT_U64('\0', "firmware-address",
>> &(cfg)->fw_addr, \
>> + "Address where firmware should be loaded"),
>>
>> #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
>> diff --git a/arm/kvm.c b/arm/kvm.c
>> index c6843e5..d5bbbc3 100644
>> --- a/arm/kvm.c
>> +++ b/arm/kvm.c
>> @@ -169,9 +169,65 @@ bool kvm__arch_load_kernel_image(struct kvm
>> *kvm, int fd_kernel, int fd_initrd, return true;
>> }
>>
>> +static bool validate_fw_addr(struct kvm *kvm)
>> +{
>> + u64 fw_addr = kvm->cfg.arch.fw_addr;
>> + u64 ram_phys;
>> +
>> + ram_phys = host_to_guest_flat(kvm, kvm->ram_start);
>> +
>> + if (fw_addr < ram_phys || fw_addr >= ram_phys +
>> kvm->ram_size) {
>> + pr_err("Provide --firmware-address an address in
>> RAM: "
>> + "0x%016llx - 0x%016llx",
>> + ram_phys, ram_phys + kvm->ram_size);
>> +
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> bool kvm__load_firmware(struct kvm *kvm, const char
>> *firmware_filename) {
>> - return false;
>> + u64 fw_addr = kvm->cfg.arch.fw_addr;
>> + void *host_pos;
>> + void *limit;
>> + ssize_t fw_sz;
>> + int fd;
>> +
>> + limit = kvm->ram_start + kvm->ram_size;
>
> What about we check here for fw_addr being 0, which is the default
> value and would be rejected by the next call.
> So can't we set it to some sensible default value here?
>
> /* If not specified, use default load address at start of RAM */
> if (fw_addr == 0)
> fw_addr = ARM_MEMORY_AREA;
>
Seems a bit random to me, but I guess we can. After all for the kernel
image we just to put it at the end of DRAM.
Thanks,
Julien
>
>> + if (!validate_fw_addr(kvm))
>> + die("Bad firmware destination: 0x%016llx", fw_addr);
>> +
>> + fd = open(firmware_filename, O_RDONLY);
>> + if (fd < 0)
>> + return false;
>> +
>> + host_pos = guest_flat_to_host(kvm, fw_addr);
>> + if (!host_pos || host_pos < kvm->ram_start)
>> + return false;
>> +
>> + fw_sz = read_file(fd, host_pos, limit - host_pos);
>> + if (fw_sz < 0)
>> + die("failed to load firmware");
>> + close(fd);
>> +
>> + /* Kernel isn't loaded by kvm, point start address to
>> firmware */
>> + kvm->arch.kern_guest_start = fw_addr;
>> +
>> + /* Load dtb just after the firmware image*/
>> + host_pos += fw_sz;
>> + if (host_pos + FDT_MAX_SIZE > limit)
>> + die("not enough space to load fdt");
>> +
>> + kvm->arch.dtb_guest_start = ALIGN(host_to_guest_flat(kvm,
>> host_pos),
>> + FDT_ALIGN);
>> + pr_info("Placing fdt at 0x%llx - 0x%llx",
>> + kvm->arch.dtb_guest_start,
>> + kvm->arch.dtb_guest_start + FDT_MAX_SIZE);
>> +
>> + return true;
>> }
>>
>> int kvm__arch_setup_firmware(struct kvm *kvm)
>
--
Julien Thierry
