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?
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;
Cheers,
Andre.
> + 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)
