* [PATCH v2 0/4] Allow loading a no MMU kernel
@ 2020-10-14 0:17 Alistair Francis
2020-10-14 0:17 ` [PATCH v2 1/4] hw/riscv: sifive_u: Allow specifying the CPU Alistair Francis
` (4 more replies)
0 siblings, 5 replies; 18+ messages in thread
From: Alistair Francis @ 2020-10-14 0:17 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair.francis, bmeng.cn, palmer, alistair23
This series allows loading a noMMU kernel using the -kernel option.
Currently if using -kernel QEMU assumes you also have firmware and loads
the kernel at a hardcoded offset. This series changes that so we only
load the kernel at an offset if a firmware (-bios) was loaded.
This series also adds a function to check if the CPU is 32-bit. This is
a step towards running 32-bit and 64-bit CPUs on the 64-bit RISC-V build
by using run time checks instead of compile time checks. We also allow
the user to sepcify a CPU for the sifive_u machine.
Alistair Francis (4):
hw/riscv: sifive_u: Allow specifying the CPU
hw/riscv: Return the end address of the loaded firmware
hw/riscv: Add a riscv_is_32_bit() function
hw/riscv: Load the kernel after the firmware
include/hw/riscv/boot.h | 13 ++++++---
include/hw/riscv/sifive_u.h | 1 +
hw/riscv/boot.c | 56 ++++++++++++++++++++++++++-----------
hw/riscv/opentitan.c | 3 +-
hw/riscv/sifive_e.c | 3 +-
hw/riscv/sifive_u.c | 28 ++++++++++++++-----
hw/riscv/spike.c | 11 ++++++--
hw/riscv/virt.c | 11 ++++++--
8 files changed, 91 insertions(+), 35 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/4] hw/riscv: sifive_u: Allow specifying the CPU
2020-10-14 0:17 [PATCH v2 0/4] Allow loading a no MMU kernel Alistair Francis
@ 2020-10-14 0:17 ` Alistair Francis
2020-10-19 23:17 ` Palmer Dabbelt
2020-10-20 3:17 ` Bin Meng
2020-10-14 0:17 ` [PATCH v2 2/4] hw/riscv: Return the end address of the loaded firmware Alistair Francis
` (3 subsequent siblings)
4 siblings, 2 replies; 18+ messages in thread
From: Alistair Francis @ 2020-10-14 0:17 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair.francis, bmeng.cn, palmer, alistair23
Allow the user to specify the main application CPU for the sifive_u
machine.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
---
include/hw/riscv/sifive_u.h | 1 +
hw/riscv/sifive_u.c | 18 +++++++++++++-----
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index 22e7e6efa1..a9f7b4a084 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -48,6 +48,7 @@ typedef struct SiFiveUSoCState {
CadenceGEMState gem;
uint32_t serial;
+ char *cpu_type;
} SiFiveUSoCState;
#define TYPE_RISCV_U_MACHINE MACHINE_TYPE_NAME("sifive_u")
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 6ad975d692..5f3ad9bc0f 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -424,6 +424,8 @@ static void sifive_u_machine_init(MachineState *machine)
object_initialize_child(OBJECT(machine), "soc", &s->soc, TYPE_RISCV_U_SOC);
object_property_set_uint(OBJECT(&s->soc), "serial", s->serial,
&error_abort);
+ object_property_set_str(OBJECT(&s->soc), "cpu-type", machine->cpu_type,
+ &error_abort);
qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
/* register RAM */
@@ -590,6 +592,11 @@ static void sifive_u_machine_class_init(ObjectClass *oc, void *data)
mc->init = sifive_u_machine_init;
mc->max_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + SIFIVE_U_COMPUTE_CPU_COUNT;
mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
+#if defined(TARGET_RISCV32)
+ mc->default_cpu_type = TYPE_RISCV_CPU_SIFIVE_U34;
+#elif defined(TARGET_RISCV64)
+ mc->default_cpu_type = TYPE_RISCV_CPU_SIFIVE_U54;
+#endif
mc->default_cpus = mc->min_cpus;
object_class_property_add_bool(oc, "start-in-flash",
@@ -618,7 +625,6 @@ type_init(sifive_u_machine_init_register_types)
static void sifive_u_soc_instance_init(Object *obj)
{
- MachineState *ms = MACHINE(qdev_get_machine());
SiFiveUSoCState *s = RISCV_U_SOC(obj);
object_initialize_child(obj, "e-cluster", &s->e_cluster, TYPE_CPU_CLUSTER);
@@ -636,10 +642,6 @@ static void sifive_u_soc_instance_init(Object *obj)
object_initialize_child(OBJECT(&s->u_cluster), "u-cpus", &s->u_cpus,
TYPE_RISCV_HART_ARRAY);
- qdev_prop_set_uint32(DEVICE(&s->u_cpus), "num-harts", ms->smp.cpus - 1);
- qdev_prop_set_uint32(DEVICE(&s->u_cpus), "hartid-base", 1);
- qdev_prop_set_string(DEVICE(&s->u_cpus), "cpu-type", SIFIVE_U_CPU);
- qdev_prop_set_uint64(DEVICE(&s->u_cpus), "resetvec", 0x1004);
object_initialize_child(obj, "prci", &s->prci, TYPE_SIFIVE_U_PRCI);
object_initialize_child(obj, "otp", &s->otp, TYPE_SIFIVE_U_OTP);
@@ -661,6 +663,11 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp)
int i;
NICInfo *nd = &nd_table[0];
+ qdev_prop_set_uint32(DEVICE(&s->u_cpus), "num-harts", ms->smp.cpus - 1);
+ qdev_prop_set_uint32(DEVICE(&s->u_cpus), "hartid-base", 1);
+ qdev_prop_set_string(DEVICE(&s->u_cpus), "cpu-type", s->cpu_type);
+ qdev_prop_set_uint64(DEVICE(&s->u_cpus), "resetvec", 0x1004);
+
sysbus_realize(SYS_BUS_DEVICE(&s->e_cpus), &error_abort);
sysbus_realize(SYS_BUS_DEVICE(&s->u_cpus), &error_abort);
/*
@@ -792,6 +799,7 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp)
static Property sifive_u_soc_props[] = {
DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
+ DEFINE_PROP_STRING("cpu-type", SiFiveUSoCState, cpu_type),
DEFINE_PROP_END_OF_LIST()
};
--
2.28.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 2/4] hw/riscv: Return the end address of the loaded firmware
2020-10-14 0:17 [PATCH v2 0/4] Allow loading a no MMU kernel Alistair Francis
2020-10-14 0:17 ` [PATCH v2 1/4] hw/riscv: sifive_u: Allow specifying the CPU Alistair Francis
@ 2020-10-14 0:17 ` Alistair Francis
2020-10-19 23:17 ` Palmer Dabbelt
2020-10-20 3:17 ` Bin Meng
2020-10-14 0:17 ` [PATCH v2 3/4] hw/riscv: Add a riscv_is_32_bit() function Alistair Francis
` (2 subsequent siblings)
4 siblings, 2 replies; 18+ messages in thread
From: Alistair Francis @ 2020-10-14 0:17 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair.francis, bmeng.cn, palmer, alistair23
Instead of returning the unused entry address from riscv_load_firmware()
instead return the end address. Also return the end address from
riscv_find_and_load_firmware().
This tells the caller if a firmware was loaded and how big it is. This
can be used to determine the load address of the next image (usually the
kernel).
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
include/hw/riscv/boot.h | 8 ++++----
hw/riscv/boot.c | 28 +++++++++++++++++-----------
2 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 451338780a..0acbd8aa6e 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -23,10 +23,10 @@
#include "exec/cpu-defs.h"
#include "hw/loader.h"
-void riscv_find_and_load_firmware(MachineState *machine,
- const char *default_machine_firmware,
- hwaddr firmware_load_addr,
- symbol_fn_t sym_cb);
+target_ulong riscv_find_and_load_firmware(MachineState *machine,
+ const char *default_machine_firmware,
+ hwaddr firmware_load_addr,
+ symbol_fn_t sym_cb);
char *riscv_find_firmware(const char *firmware_filename);
target_ulong riscv_load_firmware(const char *firmware_filename,
hwaddr firmware_load_addr,
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 21adaae56e..fa699308a0 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -40,12 +40,13 @@
#define fw_dynamic_info_data(__val) cpu_to_le64(__val)
#endif
-void riscv_find_and_load_firmware(MachineState *machine,
- const char *default_machine_firmware,
- hwaddr firmware_load_addr,
- symbol_fn_t sym_cb)
+target_ulong riscv_find_and_load_firmware(MachineState *machine,
+ const char *default_machine_firmware,
+ hwaddr firmware_load_addr,
+ symbol_fn_t sym_cb)
{
char *firmware_filename = NULL;
+ target_ulong firmware_end_addr = firmware_load_addr;
if ((!machine->firmware) || (!strcmp(machine->firmware, "default"))) {
/*
@@ -60,9 +61,12 @@ void riscv_find_and_load_firmware(MachineState *machine,
if (firmware_filename) {
/* If not "none" load the firmware */
- riscv_load_firmware(firmware_filename, firmware_load_addr, sym_cb);
+ firmware_end_addr = riscv_load_firmware(firmware_filename,
+ firmware_load_addr, sym_cb);
g_free(firmware_filename);
}
+
+ return firmware_end_addr;
}
char *riscv_find_firmware(const char *firmware_filename)
@@ -91,17 +95,19 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
hwaddr firmware_load_addr,
symbol_fn_t sym_cb)
{
- uint64_t firmware_entry;
+ uint64_t firmware_entry, firmware_size, firmware_end;
if (load_elf_ram_sym(firmware_filename, NULL, NULL, NULL,
- &firmware_entry, NULL, NULL, NULL,
+ &firmware_entry, NULL, &firmware_end, NULL,
0, EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
- return firmware_entry;
+ return firmware_end;
}
- if (load_image_targphys_as(firmware_filename, firmware_load_addr,
- ram_size, NULL) > 0) {
- return firmware_load_addr;
+ firmware_size = load_image_targphys_as(firmware_filename,
+ firmware_load_addr, ram_size, NULL);
+
+ if (firmware_size > 0) {
+ return firmware_load_addr + firmware_size;
}
error_report("could not load firmware '%s'", firmware_filename);
--
2.28.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 3/4] hw/riscv: Add a riscv_is_32_bit() function
2020-10-14 0:17 [PATCH v2 0/4] Allow loading a no MMU kernel Alistair Francis
2020-10-14 0:17 ` [PATCH v2 1/4] hw/riscv: sifive_u: Allow specifying the CPU Alistair Francis
2020-10-14 0:17 ` [PATCH v2 2/4] hw/riscv: Return the end address of the loaded firmware Alistair Francis
@ 2020-10-14 0:17 ` Alistair Francis
2020-10-19 23:17 ` Palmer Dabbelt
2020-10-20 3:17 ` Bin Meng
2020-10-14 0:17 ` [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware Alistair Francis
2020-10-20 15:44 ` [PATCH v2 0/4] Allow loading a no MMU kernel Alistair Francis
4 siblings, 2 replies; 18+ messages in thread
From: Alistair Francis @ 2020-10-14 0:17 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair.francis, bmeng.cn, palmer, alistair23
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
include/hw/riscv/boot.h | 2 ++
hw/riscv/boot.c | 9 +++++++++
2 files changed, 11 insertions(+)
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 0acbd8aa6e..2975ed1a31 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -23,6 +23,8 @@
#include "exec/cpu-defs.h"
#include "hw/loader.h"
+bool riscv_is_32_bit(MachineState *machine);
+
target_ulong riscv_find_and_load_firmware(MachineState *machine,
const char *default_machine_firmware,
hwaddr firmware_load_addr,
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index fa699308a0..5dea644f47 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -40,6 +40,15 @@
#define fw_dynamic_info_data(__val) cpu_to_le64(__val)
#endif
+bool riscv_is_32_bit(MachineState *machine)
+{
+ if (!strncmp(machine->cpu_type, "rv32", 4)) {
+ return true;
+ } else {
+ return false;
+ }
+}
+
target_ulong riscv_find_and_load_firmware(MachineState *machine,
const char *default_machine_firmware,
hwaddr firmware_load_addr,
--
2.28.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware
2020-10-14 0:17 [PATCH v2 0/4] Allow loading a no MMU kernel Alistair Francis
` (2 preceding siblings ...)
2020-10-14 0:17 ` [PATCH v2 3/4] hw/riscv: Add a riscv_is_32_bit() function Alistair Francis
@ 2020-10-14 0:17 ` Alistair Francis
2020-10-19 23:17 ` Palmer Dabbelt
2020-10-20 3:17 ` Bin Meng
2020-10-20 15:44 ` [PATCH v2 0/4] Allow loading a no MMU kernel Alistair Francis
4 siblings, 2 replies; 18+ messages in thread
From: Alistair Francis @ 2020-10-14 0:17 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair.francis, bmeng.cn, palmer, alistair23
Instead of loading the kernel at a hardcoded start address, let's load
the kernel at the next alligned address after the end of the firmware.
This should have no impact for current users of OpenSBI, but will
allow loading a noMMU kernel at the start of memory.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
include/hw/riscv/boot.h | 3 +++
hw/riscv/boot.c | 19 ++++++++++++++-----
hw/riscv/opentitan.c | 3 ++-
hw/riscv/sifive_e.c | 3 ++-
hw/riscv/sifive_u.c | 10 ++++++++--
hw/riscv/spike.c | 11 ++++++++---
hw/riscv/virt.c | 11 ++++++++---
7 files changed, 45 insertions(+), 15 deletions(-)
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 2975ed1a31..0b01988727 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -25,6 +25,8 @@
bool riscv_is_32_bit(MachineState *machine);
+target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
+ target_ulong firmware_end_addr);
target_ulong riscv_find_and_load_firmware(MachineState *machine,
const char *default_machine_firmware,
hwaddr firmware_load_addr,
@@ -34,6 +36,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
hwaddr firmware_load_addr,
symbol_fn_t sym_cb);
target_ulong riscv_load_kernel(const char *kernel_filename,
+ target_ulong firmware_end_addr,
symbol_fn_t sym_cb);
hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
uint64_t kernel_entry, hwaddr *start);
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 5dea644f47..9b3fe3fb1e 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -33,10 +33,8 @@
#include <libfdt.h>
#if defined(TARGET_RISCV32)
-# define KERNEL_BOOT_ADDRESS 0x80400000
#define fw_dynamic_info_data(__val) cpu_to_le32(__val)
#else
-# define KERNEL_BOOT_ADDRESS 0x80200000
#define fw_dynamic_info_data(__val) cpu_to_le64(__val)
#endif
@@ -49,6 +47,15 @@ bool riscv_is_32_bit(MachineState *machine)
}
}
+target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
+ target_ulong firmware_end_addr) {
+ if (riscv_is_32_bit(machine)) {
+ return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
+ } else {
+ return QEMU_ALIGN_UP(firmware_end_addr, 2 * MiB);
+ }
+}
+
target_ulong riscv_find_and_load_firmware(MachineState *machine,
const char *default_machine_firmware,
hwaddr firmware_load_addr,
@@ -123,7 +130,9 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
exit(1);
}
-target_ulong riscv_load_kernel(const char *kernel_filename, symbol_fn_t sym_cb)
+target_ulong riscv_load_kernel(const char *kernel_filename,
+ target_ulong kernel_start_addr,
+ symbol_fn_t sym_cb)
{
uint64_t kernel_entry;
@@ -138,9 +147,9 @@ target_ulong riscv_load_kernel(const char *kernel_filename, symbol_fn_t sym_cb)
return kernel_entry;
}
- if (load_image_targphys_as(kernel_filename, KERNEL_BOOT_ADDRESS,
+ if (load_image_targphys_as(kernel_filename, kernel_start_addr,
ram_size, NULL) > 0) {
- return KERNEL_BOOT_ADDRESS;
+ return kernel_start_addr;
}
error_report("could not load kernel '%s'", kernel_filename);
diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
index 0531bd879b..cc758b78b8 100644
--- a/hw/riscv/opentitan.c
+++ b/hw/riscv/opentitan.c
@@ -75,7 +75,8 @@ static void opentitan_board_init(MachineState *machine)
}
if (machine->kernel_filename) {
- riscv_load_kernel(machine->kernel_filename, NULL);
+ riscv_load_kernel(machine->kernel_filename,
+ memmap[IBEX_DEV_RAM].base, NULL);
}
}
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index fcfac16816..59bac4cc9a 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -114,7 +114,8 @@ static void sifive_e_machine_init(MachineState *machine)
memmap[SIFIVE_E_DEV_MROM].base, &address_space_memory);
if (machine->kernel_filename) {
- riscv_load_kernel(machine->kernel_filename, NULL);
+ riscv_load_kernel(machine->kernel_filename,
+ memmap[SIFIVE_E_DEV_DTIM].base, NULL);
}
}
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 5f3ad9bc0f..b2472c6627 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -415,6 +415,7 @@ static void sifive_u_machine_init(MachineState *machine)
MemoryRegion *main_mem = g_new(MemoryRegion, 1);
MemoryRegion *flash0 = g_new(MemoryRegion, 1);
target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
+ target_ulong firmware_end_addr, kernel_start_addr;
uint32_t start_addr_hi32 = 0x00000000;
int i;
uint32_t fdt_load_addr;
@@ -474,10 +475,15 @@ static void sifive_u_machine_init(MachineState *machine)
break;
}
- riscv_find_and_load_firmware(machine, BIOS_FILENAME, start_addr, NULL);
+ firmware_end_addr = riscv_find_and_load_firmware(machine, BIOS_FILENAME,
+ start_addr, NULL);
if (machine->kernel_filename) {
- kernel_entry = riscv_load_kernel(machine->kernel_filename, NULL);
+ kernel_start_addr = riscv_calc_kernel_start_addr(machine,
+ firmware_end_addr);
+
+ kernel_entry = riscv_load_kernel(machine->kernel_filename,
+ kernel_start_addr, NULL);
if (machine->initrd_filename) {
hwaddr start;
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 3fd152a035..facac6e7d2 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -195,6 +195,7 @@ static void spike_board_init(MachineState *machine)
MemoryRegion *system_memory = get_system_memory();
MemoryRegion *main_mem = g_new(MemoryRegion, 1);
MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
+ target_ulong firmware_end_addr, kernel_start_addr;
uint32_t fdt_load_addr;
uint64_t kernel_entry;
char *soc_name;
@@ -261,12 +262,16 @@ static void spike_board_init(MachineState *machine)
memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
mask_rom);
- riscv_find_and_load_firmware(machine, BIOS_FILENAME,
- memmap[SPIKE_DRAM].base,
- htif_symbol_callback);
+ firmware_end_addr = riscv_find_and_load_firmware(machine, BIOS_FILENAME,
+ memmap[SPIKE_DRAM].base,
+ htif_symbol_callback);
if (machine->kernel_filename) {
+ kernel_start_addr = riscv_calc_kernel_start_addr(machine,
+ firmware_end_addr);
+
kernel_entry = riscv_load_kernel(machine->kernel_filename,
+ kernel_start_addr,
htif_symbol_callback);
if (machine->initrd_filename) {
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 41bd2f38ba..6bfd10dfc7 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -493,6 +493,7 @@ static void virt_machine_init(MachineState *machine)
char *plic_hart_config, *soc_name;
size_t plic_hart_config_len;
target_ulong start_addr = memmap[VIRT_DRAM].base;
+ target_ulong firmware_end_addr, kernel_start_addr;
uint32_t fdt_load_addr;
uint64_t kernel_entry;
DeviceState *mmio_plic, *virtio_plic, *pcie_plic;
@@ -602,11 +603,15 @@ static void virt_machine_init(MachineState *machine)
memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
mask_rom);
- riscv_find_and_load_firmware(machine, BIOS_FILENAME,
- memmap[VIRT_DRAM].base, NULL);
+ firmware_end_addr = riscv_find_and_load_firmware(machine, BIOS_FILENAME,
+ start_addr, NULL);
if (machine->kernel_filename) {
- kernel_entry = riscv_load_kernel(machine->kernel_filename, NULL);
+ kernel_start_addr = riscv_calc_kernel_start_addr(machine,
+ firmware_end_addr);
+
+ kernel_entry = riscv_load_kernel(machine->kernel_filename,
+ kernel_start_addr, NULL);
if (machine->initrd_filename) {
hwaddr start;
--
2.28.0
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] hw/riscv: sifive_u: Allow specifying the CPU
2020-10-14 0:17 ` [PATCH v2 1/4] hw/riscv: sifive_u: Allow specifying the CPU Alistair Francis
@ 2020-10-19 23:17 ` Palmer Dabbelt
2020-10-20 3:17 ` Bin Meng
1 sibling, 0 replies; 18+ messages in thread
From: Palmer Dabbelt @ 2020-10-19 23:17 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, bmeng.cn, qemu-riscv, qemu-devel, alistair23
On Tue, 13 Oct 2020 17:17:25 PDT (-0700), Alistair Francis wrote:
> Allow the user to specify the main application CPU for the sifive_u
> machine.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
> ---
> include/hw/riscv/sifive_u.h | 1 +
> hw/riscv/sifive_u.c | 18 +++++++++++++-----
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> index 22e7e6efa1..a9f7b4a084 100644
> --- a/include/hw/riscv/sifive_u.h
> +++ b/include/hw/riscv/sifive_u.h
> @@ -48,6 +48,7 @@ typedef struct SiFiveUSoCState {
> CadenceGEMState gem;
>
> uint32_t serial;
> + char *cpu_type;
> } SiFiveUSoCState;
>
> #define TYPE_RISCV_U_MACHINE MACHINE_TYPE_NAME("sifive_u")
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 6ad975d692..5f3ad9bc0f 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -424,6 +424,8 @@ static void sifive_u_machine_init(MachineState *machine)
> object_initialize_child(OBJECT(machine), "soc", &s->soc, TYPE_RISCV_U_SOC);
> object_property_set_uint(OBJECT(&s->soc), "serial", s->serial,
> &error_abort);
> + object_property_set_str(OBJECT(&s->soc), "cpu-type", machine->cpu_type,
> + &error_abort);
> qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
>
> /* register RAM */
> @@ -590,6 +592,11 @@ static void sifive_u_machine_class_init(ObjectClass *oc, void *data)
> mc->init = sifive_u_machine_init;
> mc->max_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + SIFIVE_U_COMPUTE_CPU_COUNT;
> mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
> +#if defined(TARGET_RISCV32)
> + mc->default_cpu_type = TYPE_RISCV_CPU_SIFIVE_U34;
> +#elif defined(TARGET_RISCV64)
> + mc->default_cpu_type = TYPE_RISCV_CPU_SIFIVE_U54;
> +#endif
> mc->default_cpus = mc->min_cpus;
>
> object_class_property_add_bool(oc, "start-in-flash",
> @@ -618,7 +625,6 @@ type_init(sifive_u_machine_init_register_types)
>
> static void sifive_u_soc_instance_init(Object *obj)
> {
> - MachineState *ms = MACHINE(qdev_get_machine());
> SiFiveUSoCState *s = RISCV_U_SOC(obj);
>
> object_initialize_child(obj, "e-cluster", &s->e_cluster, TYPE_CPU_CLUSTER);
> @@ -636,10 +642,6 @@ static void sifive_u_soc_instance_init(Object *obj)
>
> object_initialize_child(OBJECT(&s->u_cluster), "u-cpus", &s->u_cpus,
> TYPE_RISCV_HART_ARRAY);
> - qdev_prop_set_uint32(DEVICE(&s->u_cpus), "num-harts", ms->smp.cpus - 1);
> - qdev_prop_set_uint32(DEVICE(&s->u_cpus), "hartid-base", 1);
> - qdev_prop_set_string(DEVICE(&s->u_cpus), "cpu-type", SIFIVE_U_CPU);
> - qdev_prop_set_uint64(DEVICE(&s->u_cpus), "resetvec", 0x1004);
>
> object_initialize_child(obj, "prci", &s->prci, TYPE_SIFIVE_U_PRCI);
> object_initialize_child(obj, "otp", &s->otp, TYPE_SIFIVE_U_OTP);
> @@ -661,6 +663,11 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp)
> int i;
> NICInfo *nd = &nd_table[0];
>
> + qdev_prop_set_uint32(DEVICE(&s->u_cpus), "num-harts", ms->smp.cpus - 1);
> + qdev_prop_set_uint32(DEVICE(&s->u_cpus), "hartid-base", 1);
> + qdev_prop_set_string(DEVICE(&s->u_cpus), "cpu-type", s->cpu_type);
> + qdev_prop_set_uint64(DEVICE(&s->u_cpus), "resetvec", 0x1004);
> +
> sysbus_realize(SYS_BUS_DEVICE(&s->e_cpus), &error_abort);
> sysbus_realize(SYS_BUS_DEVICE(&s->u_cpus), &error_abort);
> /*
> @@ -792,6 +799,7 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp)
>
> static Property sifive_u_soc_props[] = {
> DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
> + DEFINE_PROP_STRING("cpu-type", SiFiveUSoCState, cpu_type),
> DEFINE_PROP_END_OF_LIST()
> };
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/4] hw/riscv: Return the end address of the loaded firmware
2020-10-14 0:17 ` [PATCH v2 2/4] hw/riscv: Return the end address of the loaded firmware Alistair Francis
@ 2020-10-19 23:17 ` Palmer Dabbelt
2020-10-20 3:17 ` Bin Meng
1 sibling, 0 replies; 18+ messages in thread
From: Palmer Dabbelt @ 2020-10-19 23:17 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, bmeng.cn, qemu-riscv, qemu-devel, alistair23
On Tue, 13 Oct 2020 17:17:28 PDT (-0700), Alistair Francis wrote:
> Instead of returning the unused entry address from riscv_load_firmware()
> instead return the end address. Also return the end address from
> riscv_find_and_load_firmware().
>
> This tells the caller if a firmware was loaded and how big it is. This
> can be used to determine the load address of the next image (usually the
> kernel).
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> include/hw/riscv/boot.h | 8 ++++----
> hw/riscv/boot.c | 28 +++++++++++++++++-----------
> 2 files changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 451338780a..0acbd8aa6e 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -23,10 +23,10 @@
> #include "exec/cpu-defs.h"
> #include "hw/loader.h"
>
> -void riscv_find_and_load_firmware(MachineState *machine,
> - const char *default_machine_firmware,
> - hwaddr firmware_load_addr,
> - symbol_fn_t sym_cb);
> +target_ulong riscv_find_and_load_firmware(MachineState *machine,
> + const char *default_machine_firmware,
> + hwaddr firmware_load_addr,
> + symbol_fn_t sym_cb);
> char *riscv_find_firmware(const char *firmware_filename);
> target_ulong riscv_load_firmware(const char *firmware_filename,
> hwaddr firmware_load_addr,
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 21adaae56e..fa699308a0 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -40,12 +40,13 @@
> #define fw_dynamic_info_data(__val) cpu_to_le64(__val)
> #endif
>
> -void riscv_find_and_load_firmware(MachineState *machine,
> - const char *default_machine_firmware,
> - hwaddr firmware_load_addr,
> - symbol_fn_t sym_cb)
> +target_ulong riscv_find_and_load_firmware(MachineState *machine,
> + const char *default_machine_firmware,
> + hwaddr firmware_load_addr,
> + symbol_fn_t sym_cb)
> {
> char *firmware_filename = NULL;
> + target_ulong firmware_end_addr = firmware_load_addr;
>
> if ((!machine->firmware) || (!strcmp(machine->firmware, "default"))) {
> /*
> @@ -60,9 +61,12 @@ void riscv_find_and_load_firmware(MachineState *machine,
>
> if (firmware_filename) {
> /* If not "none" load the firmware */
> - riscv_load_firmware(firmware_filename, firmware_load_addr, sym_cb);
> + firmware_end_addr = riscv_load_firmware(firmware_filename,
> + firmware_load_addr, sym_cb);
> g_free(firmware_filename);
> }
> +
> + return firmware_end_addr;
> }
>
> char *riscv_find_firmware(const char *firmware_filename)
> @@ -91,17 +95,19 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
> hwaddr firmware_load_addr,
> symbol_fn_t sym_cb)
> {
> - uint64_t firmware_entry;
> + uint64_t firmware_entry, firmware_size, firmware_end;
>
> if (load_elf_ram_sym(firmware_filename, NULL, NULL, NULL,
> - &firmware_entry, NULL, NULL, NULL,
> + &firmware_entry, NULL, &firmware_end, NULL,
> 0, EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
> - return firmware_entry;
> + return firmware_end;
> }
>
> - if (load_image_targphys_as(firmware_filename, firmware_load_addr,
> - ram_size, NULL) > 0) {
> - return firmware_load_addr;
> + firmware_size = load_image_targphys_as(firmware_filename,
> + firmware_load_addr, ram_size, NULL);
> +
> + if (firmware_size > 0) {
> + return firmware_load_addr + firmware_size;
> }
>
> error_report("could not load firmware '%s'", firmware_filename);
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] hw/riscv: Add a riscv_is_32_bit() function
2020-10-14 0:17 ` [PATCH v2 3/4] hw/riscv: Add a riscv_is_32_bit() function Alistair Francis
@ 2020-10-19 23:17 ` Palmer Dabbelt
2020-10-20 3:17 ` Bin Meng
1 sibling, 0 replies; 18+ messages in thread
From: Palmer Dabbelt @ 2020-10-19 23:17 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, bmeng.cn, qemu-riscv, qemu-devel, alistair23
On Tue, 13 Oct 2020 17:17:30 PDT (-0700), Alistair Francis wrote:
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> include/hw/riscv/boot.h | 2 ++
> hw/riscv/boot.c | 9 +++++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 0acbd8aa6e..2975ed1a31 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -23,6 +23,8 @@
> #include "exec/cpu-defs.h"
> #include "hw/loader.h"
>
> +bool riscv_is_32_bit(MachineState *machine);
> +
> target_ulong riscv_find_and_load_firmware(MachineState *machine,
> const char *default_machine_firmware,
> hwaddr firmware_load_addr,
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index fa699308a0..5dea644f47 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -40,6 +40,15 @@
> #define fw_dynamic_info_data(__val) cpu_to_le64(__val)
> #endif
>
> +bool riscv_is_32_bit(MachineState *machine)
> +{
> + if (!strncmp(machine->cpu_type, "rv32", 4)) {
> + return true;
> + } else {
> + return false;
> + }
> +}
> +
> target_ulong riscv_find_and_load_firmware(MachineState *machine,
> const char *default_machine_firmware,
> hwaddr firmware_load_addr,
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware
2020-10-14 0:17 ` [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware Alistair Francis
@ 2020-10-19 23:17 ` Palmer Dabbelt
2020-10-20 15:46 ` Alistair Francis
2020-10-20 3:17 ` Bin Meng
1 sibling, 1 reply; 18+ messages in thread
From: Palmer Dabbelt @ 2020-10-19 23:17 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, bmeng.cn, qemu-riscv, qemu-devel, alistair23
On Tue, 13 Oct 2020 17:17:33 PDT (-0700), Alistair Francis wrote:
> Instead of loading the kernel at a hardcoded start address, let's load
> the kernel at the next alligned address after the end of the firmware.
>
> This should have no impact for current users of OpenSBI, but will
> allow loading a noMMU kernel at the start of memory.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> include/hw/riscv/boot.h | 3 +++
> hw/riscv/boot.c | 19 ++++++++++++++-----
> hw/riscv/opentitan.c | 3 ++-
> hw/riscv/sifive_e.c | 3 ++-
> hw/riscv/sifive_u.c | 10 ++++++++--
> hw/riscv/spike.c | 11 ++++++++---
> hw/riscv/virt.c | 11 ++++++++---
> 7 files changed, 45 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 2975ed1a31..0b01988727 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -25,6 +25,8 @@
>
> bool riscv_is_32_bit(MachineState *machine);
>
> +target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
> + target_ulong firmware_end_addr);
> target_ulong riscv_find_and_load_firmware(MachineState *machine,
> const char *default_machine_firmware,
> hwaddr firmware_load_addr,
> @@ -34,6 +36,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
> hwaddr firmware_load_addr,
> symbol_fn_t sym_cb);
> target_ulong riscv_load_kernel(const char *kernel_filename,
> + target_ulong firmware_end_addr,
> symbol_fn_t sym_cb);
> hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
> uint64_t kernel_entry, hwaddr *start);
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 5dea644f47..9b3fe3fb1e 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -33,10 +33,8 @@
> #include <libfdt.h>
>
> #if defined(TARGET_RISCV32)
> -# define KERNEL_BOOT_ADDRESS 0x80400000
> #define fw_dynamic_info_data(__val) cpu_to_le32(__val)
> #else
> -# define KERNEL_BOOT_ADDRESS 0x80200000
> #define fw_dynamic_info_data(__val) cpu_to_le64(__val)
> #endif
>
> @@ -49,6 +47,15 @@ bool riscv_is_32_bit(MachineState *machine)
> }
> }
>
> +target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
> + target_ulong firmware_end_addr) {
> + if (riscv_is_32_bit(machine)) {
> + return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
> + } else {
> + return QEMU_ALIGN_UP(firmware_end_addr, 2 * MiB);
> + }
> +}
> +
> target_ulong riscv_find_and_load_firmware(MachineState *machine,
> const char *default_machine_firmware,
> hwaddr firmware_load_addr,
> @@ -123,7 +130,9 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
> exit(1);
> }
>
> -target_ulong riscv_load_kernel(const char *kernel_filename, symbol_fn_t sym_cb)
> +target_ulong riscv_load_kernel(const char *kernel_filename,
> + target_ulong kernel_start_addr,
> + symbol_fn_t sym_cb)
> {
> uint64_t kernel_entry;
>
> @@ -138,9 +147,9 @@ target_ulong riscv_load_kernel(const char *kernel_filename, symbol_fn_t sym_cb)
> return kernel_entry;
> }
>
> - if (load_image_targphys_as(kernel_filename, KERNEL_BOOT_ADDRESS,
> + if (load_image_targphys_as(kernel_filename, kernel_start_addr,
> ram_size, NULL) > 0) {
> - return KERNEL_BOOT_ADDRESS;
> + return kernel_start_addr;
> }
>
> error_report("could not load kernel '%s'", kernel_filename);
> diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> index 0531bd879b..cc758b78b8 100644
> --- a/hw/riscv/opentitan.c
> +++ b/hw/riscv/opentitan.c
> @@ -75,7 +75,8 @@ static void opentitan_board_init(MachineState *machine)
> }
>
> if (machine->kernel_filename) {
> - riscv_load_kernel(machine->kernel_filename, NULL);
> + riscv_load_kernel(machine->kernel_filename,
> + memmap[IBEX_DEV_RAM].base, NULL);
> }
> }
>
> diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> index fcfac16816..59bac4cc9a 100644
> --- a/hw/riscv/sifive_e.c
> +++ b/hw/riscv/sifive_e.c
> @@ -114,7 +114,8 @@ static void sifive_e_machine_init(MachineState *machine)
> memmap[SIFIVE_E_DEV_MROM].base, &address_space_memory);
>
> if (machine->kernel_filename) {
> - riscv_load_kernel(machine->kernel_filename, NULL);
> + riscv_load_kernel(machine->kernel_filename,
> + memmap[SIFIVE_E_DEV_DTIM].base, NULL);
> }
> }
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 5f3ad9bc0f..b2472c6627 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -415,6 +415,7 @@ static void sifive_u_machine_init(MachineState *machine)
> MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> MemoryRegion *flash0 = g_new(MemoryRegion, 1);
> target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
> + target_ulong firmware_end_addr, kernel_start_addr;
> uint32_t start_addr_hi32 = 0x00000000;
> int i;
> uint32_t fdt_load_addr;
> @@ -474,10 +475,15 @@ static void sifive_u_machine_init(MachineState *machine)
> break;
> }
>
> - riscv_find_and_load_firmware(machine, BIOS_FILENAME, start_addr, NULL);
> + firmware_end_addr = riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> + start_addr, NULL);
>
> if (machine->kernel_filename) {
> - kernel_entry = riscv_load_kernel(machine->kernel_filename, NULL);
> + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> + firmware_end_addr);
> +
> + kernel_entry = riscv_load_kernel(machine->kernel_filename,
> + kernel_start_addr, NULL);
>
> if (machine->initrd_filename) {
> hwaddr start;
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 3fd152a035..facac6e7d2 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -195,6 +195,7 @@ static void spike_board_init(MachineState *machine)
> MemoryRegion *system_memory = get_system_memory();
> MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> + target_ulong firmware_end_addr, kernel_start_addr;
> uint32_t fdt_load_addr;
> uint64_t kernel_entry;
> char *soc_name;
> @@ -261,12 +262,16 @@ static void spike_board_init(MachineState *machine)
> memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> mask_rom);
>
> - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> - memmap[SPIKE_DRAM].base,
> - htif_symbol_callback);
> + firmware_end_addr = riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> + memmap[SPIKE_DRAM].base,
> + htif_symbol_callback);
>
> if (machine->kernel_filename) {
> + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> + firmware_end_addr);
> +
> kernel_entry = riscv_load_kernel(machine->kernel_filename,
> + kernel_start_addr,
> htif_symbol_callback);
>
> if (machine->initrd_filename) {
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 41bd2f38ba..6bfd10dfc7 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -493,6 +493,7 @@ static void virt_machine_init(MachineState *machine)
> char *plic_hart_config, *soc_name;
> size_t plic_hart_config_len;
> target_ulong start_addr = memmap[VIRT_DRAM].base;
> + target_ulong firmware_end_addr, kernel_start_addr;
> uint32_t fdt_load_addr;
> uint64_t kernel_entry;
> DeviceState *mmio_plic, *virtio_plic, *pcie_plic;
> @@ -602,11 +603,15 @@ static void virt_machine_init(MachineState *machine)
> memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
> mask_rom);
>
> - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> - memmap[VIRT_DRAM].base, NULL);
> + firmware_end_addr = riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> + start_addr, NULL);
>
> if (machine->kernel_filename) {
> - kernel_entry = riscv_load_kernel(machine->kernel_filename, NULL);
> + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> + firmware_end_addr);
> +
> + kernel_entry = riscv_load_kernel(machine->kernel_filename,
> + kernel_start_addr, NULL);
>
> if (machine->initrd_filename) {
> hwaddr start;
It's probably worth going through and making sure we mark the right regions as
reserved in the DT, as with these being mobile we might run into latent bugs.
I haven't actually looked so maybe we're fine, but IIRC we sort of papered over
a handful of memory layout agreements that weren't even in specs (or event
meant to be in specs) that have stuck around for quite a while.
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] hw/riscv: sifive_u: Allow specifying the CPU
2020-10-14 0:17 ` [PATCH v2 1/4] hw/riscv: sifive_u: Allow specifying the CPU Alistair Francis
2020-10-19 23:17 ` Palmer Dabbelt
@ 2020-10-20 3:17 ` Bin Meng
1 sibling, 0 replies; 18+ messages in thread
From: Bin Meng @ 2020-10-20 3:17 UTC (permalink / raw)
To: Alistair Francis
Cc: Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers, Alistair Francis
On Wed, Oct 14, 2020 at 8:28 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Allow the user to specify the main application CPU for the sifive_u
> machine.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
> ---
> include/hw/riscv/sifive_u.h | 1 +
> hw/riscv/sifive_u.c | 18 +++++++++++++-----
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
Tested-by: Bin Meng <bin.meng@windriver.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/4] hw/riscv: Return the end address of the loaded firmware
2020-10-14 0:17 ` [PATCH v2 2/4] hw/riscv: Return the end address of the loaded firmware Alistair Francis
2020-10-19 23:17 ` Palmer Dabbelt
@ 2020-10-20 3:17 ` Bin Meng
1 sibling, 0 replies; 18+ messages in thread
From: Bin Meng @ 2020-10-20 3:17 UTC (permalink / raw)
To: Alistair Francis
Cc: Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers, Alistair Francis
On Wed, Oct 14, 2020 at 8:28 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Instead of returning the unused entry address from riscv_load_firmware()
> instead return the end address. Also return the end address from
> riscv_find_and_load_firmware().
>
> This tells the caller if a firmware was loaded and how big it is. This
> can be used to determine the load address of the next image (usually the
> kernel).
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> include/hw/riscv/boot.h | 8 ++++----
> hw/riscv/boot.c | 28 +++++++++++++++++-----------
> 2 files changed, 21 insertions(+), 15 deletions(-)
>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
Tested-by: Bin Meng <bin.meng@windriver.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] hw/riscv: Add a riscv_is_32_bit() function
2020-10-14 0:17 ` [PATCH v2 3/4] hw/riscv: Add a riscv_is_32_bit() function Alistair Francis
2020-10-19 23:17 ` Palmer Dabbelt
@ 2020-10-20 3:17 ` Bin Meng
1 sibling, 0 replies; 18+ messages in thread
From: Bin Meng @ 2020-10-20 3:17 UTC (permalink / raw)
To: Alistair Francis
Cc: Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers, Alistair Francis
On Wed, Oct 14, 2020 at 8:28 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> include/hw/riscv/boot.h | 2 ++
> hw/riscv/boot.c | 9 +++++++++
> 2 files changed, 11 insertions(+)
>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
Tested-by: Bin Meng <bin.meng@windriver.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware
2020-10-14 0:17 ` [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware Alistair Francis
2020-10-19 23:17 ` Palmer Dabbelt
@ 2020-10-20 3:17 ` Bin Meng
1 sibling, 0 replies; 18+ messages in thread
From: Bin Meng @ 2020-10-20 3:17 UTC (permalink / raw)
To: Alistair Francis
Cc: Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers, Alistair Francis
On Wed, Oct 14, 2020 at 8:28 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Instead of loading the kernel at a hardcoded start address, let's load
> the kernel at the next alligned address after the end of the firmware.
typo of "aligned"
>
> This should have no impact for current users of OpenSBI, but will
> allow loading a noMMU kernel at the start of memory.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> include/hw/riscv/boot.h | 3 +++
> hw/riscv/boot.c | 19 ++++++++++++++-----
> hw/riscv/opentitan.c | 3 ++-
> hw/riscv/sifive_e.c | 3 ++-
> hw/riscv/sifive_u.c | 10 ++++++++--
> hw/riscv/spike.c | 11 ++++++++---
> hw/riscv/virt.c | 11 ++++++++---
> 7 files changed, 45 insertions(+), 15 deletions(-)
>
Reviewed-by: Bin Meng <bin.meng@windriver.com>
Tested-by: Bin Meng <bin.meng@windriver.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/4] Allow loading a no MMU kernel
2020-10-14 0:17 [PATCH v2 0/4] Allow loading a no MMU kernel Alistair Francis
` (3 preceding siblings ...)
2020-10-14 0:17 ` [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware Alistair Francis
@ 2020-10-20 15:44 ` Alistair Francis
4 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2020-10-20 15:44 UTC (permalink / raw)
To: Alistair Francis
Cc: Palmer Dabbelt, Bin Meng, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Tue, Oct 13, 2020 at 5:28 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> This series allows loading a noMMU kernel using the -kernel option.
> Currently if using -kernel QEMU assumes you also have firmware and loads
> the kernel at a hardcoded offset. This series changes that so we only
> load the kernel at an offset if a firmware (-bios) was loaded.
>
> This series also adds a function to check if the CPU is 32-bit. This is
> a step towards running 32-bit and 64-bit CPUs on the 64-bit RISC-V build
> by using run time checks instead of compile time checks. We also allow
> the user to sepcify a CPU for the sifive_u machine.
>
> Alistair Francis (4):
> hw/riscv: sifive_u: Allow specifying the CPU
> hw/riscv: Return the end address of the loaded firmware
> hw/riscv: Add a riscv_is_32_bit() function
> hw/riscv: Load the kernel after the firmware
Thanks!
Applied to riscv-to-apply.next
Alistair
>
> include/hw/riscv/boot.h | 13 ++++++---
> include/hw/riscv/sifive_u.h | 1 +
> hw/riscv/boot.c | 56 ++++++++++++++++++++++++++-----------
> hw/riscv/opentitan.c | 3 +-
> hw/riscv/sifive_e.c | 3 +-
> hw/riscv/sifive_u.c | 28 ++++++++++++++-----
> hw/riscv/spike.c | 11 ++++++--
> hw/riscv/virt.c | 11 ++++++--
> 8 files changed, 91 insertions(+), 35 deletions(-)
>
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware
2020-10-19 23:17 ` Palmer Dabbelt
@ 2020-10-20 15:46 ` Alistair Francis
2020-11-06 2:48 ` Palmer Dabbelt
0 siblings, 1 reply; 18+ messages in thread
From: Alistair Francis @ 2020-10-20 15:46 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: open list:RISC-V, Bin Meng, Alistair Francis,
qemu-devel@nongnu.org Developers
On Mon, Oct 19, 2020 at 4:17 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Tue, 13 Oct 2020 17:17:33 PDT (-0700), Alistair Francis wrote:
> > Instead of loading the kernel at a hardcoded start address, let's load
> > the kernel at the next alligned address after the end of the firmware.
> >
> > This should have no impact for current users of OpenSBI, but will
> > allow loading a noMMU kernel at the start of memory.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > include/hw/riscv/boot.h | 3 +++
> > hw/riscv/boot.c | 19 ++++++++++++++-----
> > hw/riscv/opentitan.c | 3 ++-
> > hw/riscv/sifive_e.c | 3 ++-
> > hw/riscv/sifive_u.c | 10 ++++++++--
> > hw/riscv/spike.c | 11 ++++++++---
> > hw/riscv/virt.c | 11 ++++++++---
> > 7 files changed, 45 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > index 2975ed1a31..0b01988727 100644
> > --- a/include/hw/riscv/boot.h
> > +++ b/include/hw/riscv/boot.h
> > @@ -25,6 +25,8 @@
> >
> > bool riscv_is_32_bit(MachineState *machine);
> >
> > +target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
> > + target_ulong firmware_end_addr);
> > target_ulong riscv_find_and_load_firmware(MachineState *machine,
> > const char *default_machine_firmware,
> > hwaddr firmware_load_addr,
> > @@ -34,6 +36,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
> > hwaddr firmware_load_addr,
> > symbol_fn_t sym_cb);
> > target_ulong riscv_load_kernel(const char *kernel_filename,
> > + target_ulong firmware_end_addr,
> > symbol_fn_t sym_cb);
> > hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
> > uint64_t kernel_entry, hwaddr *start);
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 5dea644f47..9b3fe3fb1e 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -33,10 +33,8 @@
> > #include <libfdt.h>
> >
> > #if defined(TARGET_RISCV32)
> > -# define KERNEL_BOOT_ADDRESS 0x80400000
> > #define fw_dynamic_info_data(__val) cpu_to_le32(__val)
> > #else
> > -# define KERNEL_BOOT_ADDRESS 0x80200000
> > #define fw_dynamic_info_data(__val) cpu_to_le64(__val)
> > #endif
> >
> > @@ -49,6 +47,15 @@ bool riscv_is_32_bit(MachineState *machine)
> > }
> > }
> >
> > +target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
> > + target_ulong firmware_end_addr) {
> > + if (riscv_is_32_bit(machine)) {
> > + return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
> > + } else {
> > + return QEMU_ALIGN_UP(firmware_end_addr, 2 * MiB);
> > + }
> > +}
> > +
> > target_ulong riscv_find_and_load_firmware(MachineState *machine,
> > const char *default_machine_firmware,
> > hwaddr firmware_load_addr,
> > @@ -123,7 +130,9 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
> > exit(1);
> > }
> >
> > -target_ulong riscv_load_kernel(const char *kernel_filename, symbol_fn_t sym_cb)
> > +target_ulong riscv_load_kernel(const char *kernel_filename,
> > + target_ulong kernel_start_addr,
> > + symbol_fn_t sym_cb)
> > {
> > uint64_t kernel_entry;
> >
> > @@ -138,9 +147,9 @@ target_ulong riscv_load_kernel(const char *kernel_filename, symbol_fn_t sym_cb)
> > return kernel_entry;
> > }
> >
> > - if (load_image_targphys_as(kernel_filename, KERNEL_BOOT_ADDRESS,
> > + if (load_image_targphys_as(kernel_filename, kernel_start_addr,
> > ram_size, NULL) > 0) {
> > - return KERNEL_BOOT_ADDRESS;
> > + return kernel_start_addr;
> > }
> >
> > error_report("could not load kernel '%s'", kernel_filename);
> > diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
> > index 0531bd879b..cc758b78b8 100644
> > --- a/hw/riscv/opentitan.c
> > +++ b/hw/riscv/opentitan.c
> > @@ -75,7 +75,8 @@ static void opentitan_board_init(MachineState *machine)
> > }
> >
> > if (machine->kernel_filename) {
> > - riscv_load_kernel(machine->kernel_filename, NULL);
> > + riscv_load_kernel(machine->kernel_filename,
> > + memmap[IBEX_DEV_RAM].base, NULL);
> > }
> > }
> >
> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
> > index fcfac16816..59bac4cc9a 100644
> > --- a/hw/riscv/sifive_e.c
> > +++ b/hw/riscv/sifive_e.c
> > @@ -114,7 +114,8 @@ static void sifive_e_machine_init(MachineState *machine)
> > memmap[SIFIVE_E_DEV_MROM].base, &address_space_memory);
> >
> > if (machine->kernel_filename) {
> > - riscv_load_kernel(machine->kernel_filename, NULL);
> > + riscv_load_kernel(machine->kernel_filename,
> > + memmap[SIFIVE_E_DEV_DTIM].base, NULL);
> > }
> > }
> >
> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > index 5f3ad9bc0f..b2472c6627 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -415,6 +415,7 @@ static void sifive_u_machine_init(MachineState *machine)
> > MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > MemoryRegion *flash0 = g_new(MemoryRegion, 1);
> > target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
> > + target_ulong firmware_end_addr, kernel_start_addr;
> > uint32_t start_addr_hi32 = 0x00000000;
> > int i;
> > uint32_t fdt_load_addr;
> > @@ -474,10 +475,15 @@ static void sifive_u_machine_init(MachineState *machine)
> > break;
> > }
> >
> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME, start_addr, NULL);
> > + firmware_end_addr = riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> > + start_addr, NULL);
> >
> > if (machine->kernel_filename) {
> > - kernel_entry = riscv_load_kernel(machine->kernel_filename, NULL);
> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> > + firmware_end_addr);
> > +
> > + kernel_entry = riscv_load_kernel(machine->kernel_filename,
> > + kernel_start_addr, NULL);
> >
> > if (machine->initrd_filename) {
> > hwaddr start;
> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> > index 3fd152a035..facac6e7d2 100644
> > --- a/hw/riscv/spike.c
> > +++ b/hw/riscv/spike.c
> > @@ -195,6 +195,7 @@ static void spike_board_init(MachineState *machine)
> > MemoryRegion *system_memory = get_system_memory();
> > MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > + target_ulong firmware_end_addr, kernel_start_addr;
> > uint32_t fdt_load_addr;
> > uint64_t kernel_entry;
> > char *soc_name;
> > @@ -261,12 +262,16 @@ static void spike_board_init(MachineState *machine)
> > memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> > mask_rom);
> >
> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> > - memmap[SPIKE_DRAM].base,
> > - htif_symbol_callback);
> > + firmware_end_addr = riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> > + memmap[SPIKE_DRAM].base,
> > + htif_symbol_callback);
> >
> > if (machine->kernel_filename) {
> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> > + firmware_end_addr);
> > +
> > kernel_entry = riscv_load_kernel(machine->kernel_filename,
> > + kernel_start_addr,
> > htif_symbol_callback);
> >
> > if (machine->initrd_filename) {
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 41bd2f38ba..6bfd10dfc7 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -493,6 +493,7 @@ static void virt_machine_init(MachineState *machine)
> > char *plic_hart_config, *soc_name;
> > size_t plic_hart_config_len;
> > target_ulong start_addr = memmap[VIRT_DRAM].base;
> > + target_ulong firmware_end_addr, kernel_start_addr;
> > uint32_t fdt_load_addr;
> > uint64_t kernel_entry;
> > DeviceState *mmio_plic, *virtio_plic, *pcie_plic;
> > @@ -602,11 +603,15 @@ static void virt_machine_init(MachineState *machine)
> > memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
> > mask_rom);
> >
> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> > - memmap[VIRT_DRAM].base, NULL);
> > + firmware_end_addr = riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> > + start_addr, NULL);
> >
> > if (machine->kernel_filename) {
> > - kernel_entry = riscv_load_kernel(machine->kernel_filename, NULL);
> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> > + firmware_end_addr);
> > +
> > + kernel_entry = riscv_load_kernel(machine->kernel_filename,
> > + kernel_start_addr, NULL);
> >
> > if (machine->initrd_filename) {
> > hwaddr start;
>
> It's probably worth going through and making sure we mark the right regions as
> reserved in the DT, as with these being mobile we might run into latent bugs.
Do you mean mark where the firmware is as reserved?
> I haven't actually looked so maybe we're fine, but IIRC we sort of papered over
> a handful of memory layout agreements that weren't even in specs (or event
> meant to be in specs) that have stuck around for quite a while.
For the virt machine or the sifive_u?
>
> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
Thanks
Alistair
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware
2020-10-20 15:46 ` Alistair Francis
@ 2020-11-06 2:48 ` Palmer Dabbelt
2020-11-06 4:15 ` Anup Patel
0 siblings, 1 reply; 18+ messages in thread
From: Palmer Dabbelt @ 2020-11-06 2:48 UTC (permalink / raw)
To: alistair23; +Cc: qemu-riscv, bmeng.cn, Alistair Francis, qemu-devel
On Tue, 20 Oct 2020 08:46:45 PDT (-0700), alistair23@gmail.com wrote:
> On Mon, Oct 19, 2020 at 4:17 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Tue, 13 Oct 2020 17:17:33 PDT (-0700), Alistair Francis wrote:
>> > Instead of loading the kernel at a hardcoded start address, let's load
>> > the kernel at the next alligned address after the end of the firmware.
>> >
>> > This should have no impact for current users of OpenSBI, but will
>> > allow loading a noMMU kernel at the start of memory.
>> >
>> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> > ---
>> > include/hw/riscv/boot.h | 3 +++
>> > hw/riscv/boot.c | 19 ++++++++++++++-----
>> > hw/riscv/opentitan.c | 3 ++-
>> > hw/riscv/sifive_e.c | 3 ++-
>> > hw/riscv/sifive_u.c | 10 ++++++++--
>> > hw/riscv/spike.c | 11 ++++++++---
>> > hw/riscv/virt.c | 11 ++++++++---
>> > 7 files changed, 45 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
>> > index 2975ed1a31..0b01988727 100644
>> > --- a/include/hw/riscv/boot.h
>> > +++ b/include/hw/riscv/boot.h
>> > @@ -25,6 +25,8 @@
>> >
>> > bool riscv_is_32_bit(MachineState *machine);
>> >
>> > +target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
>> > + target_ulong firmware_end_addr);
>> > target_ulong riscv_find_and_load_firmware(MachineState *machine,
>> > const char *default_machine_firmware,
>> > hwaddr firmware_load_addr,
>> > @@ -34,6 +36,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>> > hwaddr firmware_load_addr,
>> > symbol_fn_t sym_cb);
>> > target_ulong riscv_load_kernel(const char *kernel_filename,
>> > + target_ulong firmware_end_addr,
>> > symbol_fn_t sym_cb);
>> > hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size,
>> > uint64_t kernel_entry, hwaddr *start);
>> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>> > index 5dea644f47..9b3fe3fb1e 100644
>> > --- a/hw/riscv/boot.c
>> > +++ b/hw/riscv/boot.c
>> > @@ -33,10 +33,8 @@
>> > #include <libfdt.h>
>> >
>> > #if defined(TARGET_RISCV32)
>> > -# define KERNEL_BOOT_ADDRESS 0x80400000
>> > #define fw_dynamic_info_data(__val) cpu_to_le32(__val)
>> > #else
>> > -# define KERNEL_BOOT_ADDRESS 0x80200000
>> > #define fw_dynamic_info_data(__val) cpu_to_le64(__val)
>> > #endif
>> >
>> > @@ -49,6 +47,15 @@ bool riscv_is_32_bit(MachineState *machine)
>> > }
>> > }
>> >
>> > +target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
>> > + target_ulong firmware_end_addr) {
>> > + if (riscv_is_32_bit(machine)) {
>> > + return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
>> > + } else {
>> > + return QEMU_ALIGN_UP(firmware_end_addr, 2 * MiB);
>> > + }
>> > +}
>> > +
>> > target_ulong riscv_find_and_load_firmware(MachineState *machine,
>> > const char *default_machine_firmware,
>> > hwaddr firmware_load_addr,
>> > @@ -123,7 +130,9 @@ target_ulong riscv_load_firmware(const char *firmware_filename,
>> > exit(1);
>> > }
>> >
>> > -target_ulong riscv_load_kernel(const char *kernel_filename, symbol_fn_t sym_cb)
>> > +target_ulong riscv_load_kernel(const char *kernel_filename,
>> > + target_ulong kernel_start_addr,
>> > + symbol_fn_t sym_cb)
>> > {
>> > uint64_t kernel_entry;
>> >
>> > @@ -138,9 +147,9 @@ target_ulong riscv_load_kernel(const char *kernel_filename, symbol_fn_t sym_cb)
>> > return kernel_entry;
>> > }
>> >
>> > - if (load_image_targphys_as(kernel_filename, KERNEL_BOOT_ADDRESS,
>> > + if (load_image_targphys_as(kernel_filename, kernel_start_addr,
>> > ram_size, NULL) > 0) {
>> > - return KERNEL_BOOT_ADDRESS;
>> > + return kernel_start_addr;
>> > }
>> >
>> > error_report("could not load kernel '%s'", kernel_filename);
>> > diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c
>> > index 0531bd879b..cc758b78b8 100644
>> > --- a/hw/riscv/opentitan.c
>> > +++ b/hw/riscv/opentitan.c
>> > @@ -75,7 +75,8 @@ static void opentitan_board_init(MachineState *machine)
>> > }
>> >
>> > if (machine->kernel_filename) {
>> > - riscv_load_kernel(machine->kernel_filename, NULL);
>> > + riscv_load_kernel(machine->kernel_filename,
>> > + memmap[IBEX_DEV_RAM].base, NULL);
>> > }
>> > }
>> >
>> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
>> > index fcfac16816..59bac4cc9a 100644
>> > --- a/hw/riscv/sifive_e.c
>> > +++ b/hw/riscv/sifive_e.c
>> > @@ -114,7 +114,8 @@ static void sifive_e_machine_init(MachineState *machine)
>> > memmap[SIFIVE_E_DEV_MROM].base, &address_space_memory);
>> >
>> > if (machine->kernel_filename) {
>> > - riscv_load_kernel(machine->kernel_filename, NULL);
>> > + riscv_load_kernel(machine->kernel_filename,
>> > + memmap[SIFIVE_E_DEV_DTIM].base, NULL);
>> > }
>> > }
>> >
>> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
>> > index 5f3ad9bc0f..b2472c6627 100644
>> > --- a/hw/riscv/sifive_u.c
>> > +++ b/hw/riscv/sifive_u.c
>> > @@ -415,6 +415,7 @@ static void sifive_u_machine_init(MachineState *machine)
>> > MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>> > MemoryRegion *flash0 = g_new(MemoryRegion, 1);
>> > target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
>> > + target_ulong firmware_end_addr, kernel_start_addr;
>> > uint32_t start_addr_hi32 = 0x00000000;
>> > int i;
>> > uint32_t fdt_load_addr;
>> > @@ -474,10 +475,15 @@ static void sifive_u_machine_init(MachineState *machine)
>> > break;
>> > }
>> >
>> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME, start_addr, NULL);
>> > + firmware_end_addr = riscv_find_and_load_firmware(machine, BIOS_FILENAME,
>> > + start_addr, NULL);
>> >
>> > if (machine->kernel_filename) {
>> > - kernel_entry = riscv_load_kernel(machine->kernel_filename, NULL);
>> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
>> > + firmware_end_addr);
>> > +
>> > + kernel_entry = riscv_load_kernel(machine->kernel_filename,
>> > + kernel_start_addr, NULL);
>> >
>> > if (machine->initrd_filename) {
>> > hwaddr start;
>> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> > index 3fd152a035..facac6e7d2 100644
>> > --- a/hw/riscv/spike.c
>> > +++ b/hw/riscv/spike.c
>> > @@ -195,6 +195,7 @@ static void spike_board_init(MachineState *machine)
>> > MemoryRegion *system_memory = get_system_memory();
>> > MemoryRegion *main_mem = g_new(MemoryRegion, 1);
>> > MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
>> > + target_ulong firmware_end_addr, kernel_start_addr;
>> > uint32_t fdt_load_addr;
>> > uint64_t kernel_entry;
>> > char *soc_name;
>> > @@ -261,12 +262,16 @@ static void spike_board_init(MachineState *machine)
>> > memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
>> > mask_rom);
>> >
>> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
>> > - memmap[SPIKE_DRAM].base,
>> > - htif_symbol_callback);
>> > + firmware_end_addr = riscv_find_and_load_firmware(machine, BIOS_FILENAME,
>> > + memmap[SPIKE_DRAM].base,
>> > + htif_symbol_callback);
>> >
>> > if (machine->kernel_filename) {
>> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
>> > + firmware_end_addr);
>> > +
>> > kernel_entry = riscv_load_kernel(machine->kernel_filename,
>> > + kernel_start_addr,
>> > htif_symbol_callback);
>> >
>> > if (machine->initrd_filename) {
>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> > index 41bd2f38ba..6bfd10dfc7 100644
>> > --- a/hw/riscv/virt.c
>> > +++ b/hw/riscv/virt.c
>> > @@ -493,6 +493,7 @@ static void virt_machine_init(MachineState *machine)
>> > char *plic_hart_config, *soc_name;
>> > size_t plic_hart_config_len;
>> > target_ulong start_addr = memmap[VIRT_DRAM].base;
>> > + target_ulong firmware_end_addr, kernel_start_addr;
>> > uint32_t fdt_load_addr;
>> > uint64_t kernel_entry;
>> > DeviceState *mmio_plic, *virtio_plic, *pcie_plic;
>> > @@ -602,11 +603,15 @@ static void virt_machine_init(MachineState *machine)
>> > memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
>> > mask_rom);
>> >
>> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
>> > - memmap[VIRT_DRAM].base, NULL);
>> > + firmware_end_addr = riscv_find_and_load_firmware(machine, BIOS_FILENAME,
>> > + start_addr, NULL);
>> >
>> > if (machine->kernel_filename) {
>> > - kernel_entry = riscv_load_kernel(machine->kernel_filename, NULL);
>> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
>> > + firmware_end_addr);
>> > +
>> > + kernel_entry = riscv_load_kernel(machine->kernel_filename,
>> > + kernel_start_addr, NULL);
>> >
>> > if (machine->initrd_filename) {
>> > hwaddr start;
>>
>> It's probably worth going through and making sure we mark the right regions as
>> reserved in the DT, as with these being mobile we might run into latent bugs.
>
> Do you mean mark where the firmware is as reserved?
Ya. We have some things like Linux assuming that the first page in memory is
reserved for the bootloader, which IIRC never really got written down in any
specifications. That's an implicit S-mode ABI, so it doesn't apply directly,
but I'd guess there's some of this floating around elsewhere in the stack.
>> I haven't actually looked so maybe we're fine, but IIRC we sort of papered over
>> a handful of memory layout agreements that weren't even in specs (or event
>> meant to be in specs) that have stuck around for quite a while.
>
> For the virt machine or the sifive_u?
My guess is that any of this would apply to both of these, as the issue would
be assumptions within the firmware about the memory image provided to it when
it's entered. That would be an ABI between the firmware and whatever loads it,
but IIRC for both the virt board and the sifive_u we assume the firmware is
just loaded directly my QEMU.
The sifive_u board may be additionally constrained by SiFive's boot ROM, but I
don't remember it really caring about any of this. Also not sure it's even
used any more, as it was really just a vehicle to demonstrate initializing the
widgets for which we couldn't release RTL.
>> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
>
> Thanks
>
> Alistair
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware
2020-11-06 2:48 ` Palmer Dabbelt
@ 2020-11-06 4:15 ` Anup Patel
2020-11-09 23:19 ` Alistair Francis
0 siblings, 1 reply; 18+ messages in thread
From: Anup Patel @ 2020-11-06 4:15 UTC (permalink / raw)
To: Palmer Dabbelt, alistair23
Cc: Alistair Francis, bmeng.cn, qemu-riscv, qemu-devel
> -----Original Message-----
> From: Qemu-riscv <qemu-riscv-
> bounces+anup.patel=wdc.com@nongnu.org> On Behalf Of Palmer Dabbelt
> Sent: 06 November 2020 08:19
> To: alistair23@gmail.com
> Cc: qemu-riscv@nongnu.org; bmeng.cn@gmail.com; Alistair Francis
> <Alistair.Francis@wdc.com>; qemu-devel@nongnu.org
> Subject: Re: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware
>
> On Tue, 20 Oct 2020 08:46:45 PDT (-0700), alistair23@gmail.com wrote:
> > On Mon, Oct 19, 2020 at 4:17 PM Palmer Dabbelt <palmer@dabbelt.com>
> wrote:
> >>
> >> On Tue, 13 Oct 2020 17:17:33 PDT (-0700), Alistair Francis wrote:
> >> > Instead of loading the kernel at a hardcoded start address, let's
> >> > load the kernel at the next alligned address after the end of the
> firmware.
> >> >
> >> > This should have no impact for current users of OpenSBI, but will
> >> > allow loading a noMMU kernel at the start of memory.
> >> >
> >> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >> > ---
> >> > include/hw/riscv/boot.h | 3 +++
> >> > hw/riscv/boot.c | 19 ++++++++++++++-----
> >> > hw/riscv/opentitan.c | 3 ++-
> >> > hw/riscv/sifive_e.c | 3 ++-
> >> > hw/riscv/sifive_u.c | 10 ++++++++--
> >> > hw/riscv/spike.c | 11 ++++++++---
> >> > hw/riscv/virt.c | 11 ++++++++---
> >> > 7 files changed, 45 insertions(+), 15 deletions(-)
> >> >
> >> > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> >> > index 2975ed1a31..0b01988727 100644
> >> > --- a/include/hw/riscv/boot.h
> >> > +++ b/include/hw/riscv/boot.h
> >> > @@ -25,6 +25,8 @@
> >> >
> >> > bool riscv_is_32_bit(MachineState *machine);
> >> >
> >> > +target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
> >> > + target_ulong
> >> > +firmware_end_addr);
> >> > target_ulong riscv_find_and_load_firmware(MachineState *machine,
> >> > const char *default_machine_firmware,
> >> > hwaddr
> >> > firmware_load_addr, @@ -34,6 +36,7 @@ target_ulong
> riscv_load_firmware(const char *firmware_filename,
> >> > hwaddr firmware_load_addr,
> >> > symbol_fn_t sym_cb);
> >> > target_ulong riscv_load_kernel(const char *kernel_filename,
> >> > + target_ulong firmware_end_addr,
> >> > symbol_fn_t sym_cb); hwaddr
> >> > riscv_load_initrd(const char *filename, uint64_t mem_size,
> >> > uint64_t kernel_entry, hwaddr *start);
> >> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c index
> >> > 5dea644f47..9b3fe3fb1e 100644
> >> > --- a/hw/riscv/boot.c
> >> > +++ b/hw/riscv/boot.c
> >> > @@ -33,10 +33,8 @@
> >> > #include <libfdt.h>
> >> >
> >> > #if defined(TARGET_RISCV32)
> >> > -# define KERNEL_BOOT_ADDRESS 0x80400000
> >> > #define fw_dynamic_info_data(__val) cpu_to_le32(__val)
> >> > #else
> >> > -# define KERNEL_BOOT_ADDRESS 0x80200000
> >> > #define fw_dynamic_info_data(__val) cpu_to_le64(__val)
> >> > #endif
> >> >
> >> > @@ -49,6 +47,15 @@ bool riscv_is_32_bit(MachineState *machine)
> >> > }
> >> > }
> >> >
> >> > +target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
> >> > + target_ulong firmware_end_addr) {
> >> > + if (riscv_is_32_bit(machine)) {
> >> > + return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
> >> > + } else {
> >> > + return QEMU_ALIGN_UP(firmware_end_addr, 2 * MiB);
> >> > + }
> >> > +}
> >> > +
> >> > target_ulong riscv_find_and_load_firmware(MachineState *machine,
> >> > const char *default_machine_firmware,
> >> > hwaddr
> >> > firmware_load_addr, @@ -123,7 +130,9 @@ target_ulong
> riscv_load_firmware(const char *firmware_filename,
> >> > exit(1);
> >> > }
> >> >
> >> > -target_ulong riscv_load_kernel(const char *kernel_filename,
> >> > symbol_fn_t sym_cb)
> >> > +target_ulong riscv_load_kernel(const char *kernel_filename,
> >> > + target_ulong kernel_start_addr,
> >> > + symbol_fn_t sym_cb)
> >> > {
> >> > uint64_t kernel_entry;
> >> >
> >> > @@ -138,9 +147,9 @@ target_ulong riscv_load_kernel(const char
> *kernel_filename, symbol_fn_t sym_cb)
> >> > return kernel_entry;
> >> > }
> >> >
> >> > - if (load_image_targphys_as(kernel_filename,
> KERNEL_BOOT_ADDRESS,
> >> > + if (load_image_targphys_as(kernel_filename, kernel_start_addr,
> >> > ram_size, NULL) > 0) {
> >> > - return KERNEL_BOOT_ADDRESS;
> >> > + return kernel_start_addr;
> >> > }
> >> >
> >> > error_report("could not load kernel '%s'", kernel_filename);
> >> > diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c index
> >> > 0531bd879b..cc758b78b8 100644
> >> > --- a/hw/riscv/opentitan.c
> >> > +++ b/hw/riscv/opentitan.c
> >> > @@ -75,7 +75,8 @@ static void opentitan_board_init(MachineState
> *machine)
> >> > }
> >> >
> >> > if (machine->kernel_filename) {
> >> > - riscv_load_kernel(machine->kernel_filename, NULL);
> >> > + riscv_load_kernel(machine->kernel_filename,
> >> > + memmap[IBEX_DEV_RAM].base, NULL);
> >> > }
> >> > }
> >> >
> >> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c index
> >> > fcfac16816..59bac4cc9a 100644
> >> > --- a/hw/riscv/sifive_e.c
> >> > +++ b/hw/riscv/sifive_e.c
> >> > @@ -114,7 +114,8 @@ static void sifive_e_machine_init(MachineState
> *machine)
> >> > memmap[SIFIVE_E_DEV_MROM].base,
> >> > &address_space_memory);
> >> >
> >> > if (machine->kernel_filename) {
> >> > - riscv_load_kernel(machine->kernel_filename, NULL);
> >> > + riscv_load_kernel(machine->kernel_filename,
> >> > + memmap[SIFIVE_E_DEV_DTIM].base, NULL);
> >> > }
> >> > }
> >> >
> >> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index
> >> > 5f3ad9bc0f..b2472c6627 100644
> >> > --- a/hw/riscv/sifive_u.c
> >> > +++ b/hw/riscv/sifive_u.c
> >> > @@ -415,6 +415,7 @@ static void sifive_u_machine_init(MachineState
> *machine)
> >> > MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >> > MemoryRegion *flash0 = g_new(MemoryRegion, 1);
> >> > target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
> >> > + target_ulong firmware_end_addr, kernel_start_addr;
> >> > uint32_t start_addr_hi32 = 0x00000000;
> >> > int i;
> >> > uint32_t fdt_load_addr;
> >> > @@ -474,10 +475,15 @@ static void
> sifive_u_machine_init(MachineState *machine)
> >> > break;
> >> > }
> >> >
> >> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> start_addr, NULL);
> >> > + firmware_end_addr = riscv_find_and_load_firmware(machine,
> BIOS_FILENAME,
> >> > + start_addr,
> >> > + NULL);
> >> >
> >> > if (machine->kernel_filename) {
> >> > - kernel_entry = riscv_load_kernel(machine->kernel_filename,
> NULL);
> >> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> >> > +
> >> > + firmware_end_addr);
> >> > +
> >> > + kernel_entry = riscv_load_kernel(machine->kernel_filename,
> >> > + kernel_start_addr, NULL);
> >> >
> >> > if (machine->initrd_filename) {
> >> > hwaddr start;
> >> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index
> >> > 3fd152a035..facac6e7d2 100644
> >> > --- a/hw/riscv/spike.c
> >> > +++ b/hw/riscv/spike.c
> >> > @@ -195,6 +195,7 @@ static void spike_board_init(MachineState
> *machine)
> >> > MemoryRegion *system_memory = get_system_memory();
> >> > MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> >> > MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> >> > + target_ulong firmware_end_addr, kernel_start_addr;
> >> > uint32_t fdt_load_addr;
> >> > uint64_t kernel_entry;
> >> > char *soc_name;
> >> > @@ -261,12 +262,16 @@ static void spike_board_init(MachineState
> *machine)
> >> > memory_region_add_subregion(system_memory,
> memmap[SPIKE_MROM].base,
> >> > mask_rom);
> >> >
> >> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> >> > - memmap[SPIKE_DRAM].base,
> >> > - htif_symbol_callback);
> >> > + firmware_end_addr = riscv_find_and_load_firmware(machine,
> BIOS_FILENAME,
> >> > + memmap[SPIKE_DRAM].base,
> >> > +
> >> > + htif_symbol_callback);
> >> >
> >> > if (machine->kernel_filename) {
> >> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> >> > +
> >> > + firmware_end_addr);
> >> > +
> >> > kernel_entry = riscv_load_kernel(machine->kernel_filename,
> >> > + kernel_start_addr,
> >> > htif_symbol_callback);
> >> >
> >> > if (machine->initrd_filename) { diff --git
> >> > a/hw/riscv/virt.c b/hw/riscv/virt.c index 41bd2f38ba..6bfd10dfc7
> >> > 100644
> >> > --- a/hw/riscv/virt.c
> >> > +++ b/hw/riscv/virt.c
> >> > @@ -493,6 +493,7 @@ static void virt_machine_init(MachineState
> *machine)
> >> > char *plic_hart_config, *soc_name;
> >> > size_t plic_hart_config_len;
> >> > target_ulong start_addr = memmap[VIRT_DRAM].base;
> >> > + target_ulong firmware_end_addr, kernel_start_addr;
> >> > uint32_t fdt_load_addr;
> >> > uint64_t kernel_entry;
> >> > DeviceState *mmio_plic, *virtio_plic, *pcie_plic; @@ -602,11
> >> > +603,15 @@ static void virt_machine_init(MachineState *machine)
> >> > memory_region_add_subregion(system_memory,
> memmap[VIRT_MROM].base,
> >> > mask_rom);
> >> >
> >> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> >> > - memmap[VIRT_DRAM].base, NULL);
> >> > + firmware_end_addr = riscv_find_and_load_firmware(machine,
> BIOS_FILENAME,
> >> > + start_addr,
> >> > + NULL);
> >> >
> >> > if (machine->kernel_filename) {
> >> > - kernel_entry = riscv_load_kernel(machine->kernel_filename,
> NULL);
> >> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> >> > +
> >> > + firmware_end_addr);
> >> > +
> >> > + kernel_entry = riscv_load_kernel(machine->kernel_filename,
> >> > + kernel_start_addr, NULL);
> >> >
> >> > if (machine->initrd_filename) {
> >> > hwaddr start;
> >>
> >> It's probably worth going through and making sure we mark the right
> >> regions as reserved in the DT, as with these being mobile we might run
> into latent bugs.
> >
> > Do you mean mark where the firmware is as reserved?
>
> Ya. We have some things like Linux assuming that the first page in memory is
> reserved for the bootloader, which IIRC never really got written down in any
> specifications. That's an implicit S-mode ABI, so it doesn't apply directly, but
> I'd guess there's some of this floating around elsewhere in the stack.
Let's not do any DT reservation based on firmware load location because
firmwares can relocate itself to some other address. Further, firmware such
as OpenSBI can now create domains where only firmware knows how the
system is partitioned into domains and do DT reservations accordingly.
I suggest we let firmware add the required reservations in DT
>
> >> I haven't actually looked so maybe we're fine, but IIRC we sort of
> >> papered over a handful of memory layout agreements that weren't even
> >> in specs (or event meant to be in specs) that have stuck around for quite a
> while.
> >
> > For the virt machine or the sifive_u?
>
> My guess is that any of this would apply to both of these, as the issue would
> be assumptions within the firmware about the memory image provided to it
> when it's entered. That would be an ABI between the firmware and
> whatever loads it, but IIRC for both the virt board and the sifive_u we
> assume the firmware is just loaded directly my QEMU.
>
> The sifive_u board may be additionally constrained by SiFive's boot ROM, but
> I don't remember it really caring about any of this. Also not sure it's even
> used any more, as it was really just a vehicle to demonstrate initializing the
> widgets for which we couldn't release RTL.
>
> >> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
> >
> > Thanks
> >
> > Alistair
Regards,
Anup
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware
2020-11-06 4:15 ` Anup Patel
@ 2020-11-09 23:19 ` Alistair Francis
0 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2020-11-09 23:19 UTC (permalink / raw)
To: Anup Patel
Cc: qemu-riscv, Alistair Francis, bmeng.cn, Palmer Dabbelt, qemu-devel
On Thu, Nov 5, 2020 at 8:15 PM Anup Patel <Anup.Patel@wdc.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Qemu-riscv <qemu-riscv-
> > bounces+anup.patel=wdc.com@nongnu.org> On Behalf Of Palmer Dabbelt
> > Sent: 06 November 2020 08:19
> > To: alistair23@gmail.com
> > Cc: qemu-riscv@nongnu.org; bmeng.cn@gmail.com; Alistair Francis
> > <Alistair.Francis@wdc.com>; qemu-devel@nongnu.org
> > Subject: Re: [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware
> >
> > On Tue, 20 Oct 2020 08:46:45 PDT (-0700), alistair23@gmail.com wrote:
> > > On Mon, Oct 19, 2020 at 4:17 PM Palmer Dabbelt <palmer@dabbelt.com>
> > wrote:
> > >>
> > >> On Tue, 13 Oct 2020 17:17:33 PDT (-0700), Alistair Francis wrote:
> > >> > Instead of loading the kernel at a hardcoded start address, let's
> > >> > load the kernel at the next alligned address after the end of the
> > firmware.
> > >> >
> > >> > This should have no impact for current users of OpenSBI, but will
> > >> > allow loading a noMMU kernel at the start of memory.
> > >> >
> > >> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > >> > ---
> > >> > include/hw/riscv/boot.h | 3 +++
> > >> > hw/riscv/boot.c | 19 ++++++++++++++-----
> > >> > hw/riscv/opentitan.c | 3 ++-
> > >> > hw/riscv/sifive_e.c | 3 ++-
> > >> > hw/riscv/sifive_u.c | 10 ++++++++--
> > >> > hw/riscv/spike.c | 11 ++++++++---
> > >> > hw/riscv/virt.c | 11 ++++++++---
> > >> > 7 files changed, 45 insertions(+), 15 deletions(-)
> > >> >
> > >> > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > >> > index 2975ed1a31..0b01988727 100644
> > >> > --- a/include/hw/riscv/boot.h
> > >> > +++ b/include/hw/riscv/boot.h
> > >> > @@ -25,6 +25,8 @@
> > >> >
> > >> > bool riscv_is_32_bit(MachineState *machine);
> > >> >
> > >> > +target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
> > >> > + target_ulong
> > >> > +firmware_end_addr);
> > >> > target_ulong riscv_find_and_load_firmware(MachineState *machine,
> > >> > const char *default_machine_firmware,
> > >> > hwaddr
> > >> > firmware_load_addr, @@ -34,6 +36,7 @@ target_ulong
> > riscv_load_firmware(const char *firmware_filename,
> > >> > hwaddr firmware_load_addr,
> > >> > symbol_fn_t sym_cb);
> > >> > target_ulong riscv_load_kernel(const char *kernel_filename,
> > >> > + target_ulong firmware_end_addr,
> > >> > symbol_fn_t sym_cb); hwaddr
> > >> > riscv_load_initrd(const char *filename, uint64_t mem_size,
> > >> > uint64_t kernel_entry, hwaddr *start);
> > >> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c index
> > >> > 5dea644f47..9b3fe3fb1e 100644
> > >> > --- a/hw/riscv/boot.c
> > >> > +++ b/hw/riscv/boot.c
> > >> > @@ -33,10 +33,8 @@
> > >> > #include <libfdt.h>
> > >> >
> > >> > #if defined(TARGET_RISCV32)
> > >> > -# define KERNEL_BOOT_ADDRESS 0x80400000
> > >> > #define fw_dynamic_info_data(__val) cpu_to_le32(__val)
> > >> > #else
> > >> > -# define KERNEL_BOOT_ADDRESS 0x80200000
> > >> > #define fw_dynamic_info_data(__val) cpu_to_le64(__val)
> > >> > #endif
> > >> >
> > >> > @@ -49,6 +47,15 @@ bool riscv_is_32_bit(MachineState *machine)
> > >> > }
> > >> > }
> > >> >
> > >> > +target_ulong riscv_calc_kernel_start_addr(MachineState *machine,
> > >> > + target_ulong firmware_end_addr) {
> > >> > + if (riscv_is_32_bit(machine)) {
> > >> > + return QEMU_ALIGN_UP(firmware_end_addr, 4 * MiB);
> > >> > + } else {
> > >> > + return QEMU_ALIGN_UP(firmware_end_addr, 2 * MiB);
> > >> > + }
> > >> > +}
> > >> > +
> > >> > target_ulong riscv_find_and_load_firmware(MachineState *machine,
> > >> > const char *default_machine_firmware,
> > >> > hwaddr
> > >> > firmware_load_addr, @@ -123,7 +130,9 @@ target_ulong
> > riscv_load_firmware(const char *firmware_filename,
> > >> > exit(1);
> > >> > }
> > >> >
> > >> > -target_ulong riscv_load_kernel(const char *kernel_filename,
> > >> > symbol_fn_t sym_cb)
> > >> > +target_ulong riscv_load_kernel(const char *kernel_filename,
> > >> > + target_ulong kernel_start_addr,
> > >> > + symbol_fn_t sym_cb)
> > >> > {
> > >> > uint64_t kernel_entry;
> > >> >
> > >> > @@ -138,9 +147,9 @@ target_ulong riscv_load_kernel(const char
> > *kernel_filename, symbol_fn_t sym_cb)
> > >> > return kernel_entry;
> > >> > }
> > >> >
> > >> > - if (load_image_targphys_as(kernel_filename,
> > KERNEL_BOOT_ADDRESS,
> > >> > + if (load_image_targphys_as(kernel_filename, kernel_start_addr,
> > >> > ram_size, NULL) > 0) {
> > >> > - return KERNEL_BOOT_ADDRESS;
> > >> > + return kernel_start_addr;
> > >> > }
> > >> >
> > >> > error_report("could not load kernel '%s'", kernel_filename);
> > >> > diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c index
> > >> > 0531bd879b..cc758b78b8 100644
> > >> > --- a/hw/riscv/opentitan.c
> > >> > +++ b/hw/riscv/opentitan.c
> > >> > @@ -75,7 +75,8 @@ static void opentitan_board_init(MachineState
> > *machine)
> > >> > }
> > >> >
> > >> > if (machine->kernel_filename) {
> > >> > - riscv_load_kernel(machine->kernel_filename, NULL);
> > >> > + riscv_load_kernel(machine->kernel_filename,
> > >> > + memmap[IBEX_DEV_RAM].base, NULL);
> > >> > }
> > >> > }
> > >> >
> > >> > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c index
> > >> > fcfac16816..59bac4cc9a 100644
> > >> > --- a/hw/riscv/sifive_e.c
> > >> > +++ b/hw/riscv/sifive_e.c
> > >> > @@ -114,7 +114,8 @@ static void sifive_e_machine_init(MachineState
> > *machine)
> > >> > memmap[SIFIVE_E_DEV_MROM].base,
> > >> > &address_space_memory);
> > >> >
> > >> > if (machine->kernel_filename) {
> > >> > - riscv_load_kernel(machine->kernel_filename, NULL);
> > >> > + riscv_load_kernel(machine->kernel_filename,
> > >> > + memmap[SIFIVE_E_DEV_DTIM].base, NULL);
> > >> > }
> > >> > }
> > >> >
> > >> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index
> > >> > 5f3ad9bc0f..b2472c6627 100644
> > >> > --- a/hw/riscv/sifive_u.c
> > >> > +++ b/hw/riscv/sifive_u.c
> > >> > @@ -415,6 +415,7 @@ static void sifive_u_machine_init(MachineState
> > *machine)
> > >> > MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > >> > MemoryRegion *flash0 = g_new(MemoryRegion, 1);
> > >> > target_ulong start_addr = memmap[SIFIVE_U_DEV_DRAM].base;
> > >> > + target_ulong firmware_end_addr, kernel_start_addr;
> > >> > uint32_t start_addr_hi32 = 0x00000000;
> > >> > int i;
> > >> > uint32_t fdt_load_addr;
> > >> > @@ -474,10 +475,15 @@ static void
> > sifive_u_machine_init(MachineState *machine)
> > >> > break;
> > >> > }
> > >> >
> > >> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> > start_addr, NULL);
> > >> > + firmware_end_addr = riscv_find_and_load_firmware(machine,
> > BIOS_FILENAME,
> > >> > + start_addr,
> > >> > + NULL);
> > >> >
> > >> > if (machine->kernel_filename) {
> > >> > - kernel_entry = riscv_load_kernel(machine->kernel_filename,
> > NULL);
> > >> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> > >> > +
> > >> > + firmware_end_addr);
> > >> > +
> > >> > + kernel_entry = riscv_load_kernel(machine->kernel_filename,
> > >> > + kernel_start_addr, NULL);
> > >> >
> > >> > if (machine->initrd_filename) {
> > >> > hwaddr start;
> > >> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index
> > >> > 3fd152a035..facac6e7d2 100644
> > >> > --- a/hw/riscv/spike.c
> > >> > +++ b/hw/riscv/spike.c
> > >> > @@ -195,6 +195,7 @@ static void spike_board_init(MachineState
> > *machine)
> > >> > MemoryRegion *system_memory = get_system_memory();
> > >> > MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > >> > MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> > >> > + target_ulong firmware_end_addr, kernel_start_addr;
> > >> > uint32_t fdt_load_addr;
> > >> > uint64_t kernel_entry;
> > >> > char *soc_name;
> > >> > @@ -261,12 +262,16 @@ static void spike_board_init(MachineState
> > *machine)
> > >> > memory_region_add_subregion(system_memory,
> > memmap[SPIKE_MROM].base,
> > >> > mask_rom);
> > >> >
> > >> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> > >> > - memmap[SPIKE_DRAM].base,
> > >> > - htif_symbol_callback);
> > >> > + firmware_end_addr = riscv_find_and_load_firmware(machine,
> > BIOS_FILENAME,
> > >> > + memmap[SPIKE_DRAM].base,
> > >> > +
> > >> > + htif_symbol_callback);
> > >> >
> > >> > if (machine->kernel_filename) {
> > >> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> > >> > +
> > >> > + firmware_end_addr);
> > >> > +
> > >> > kernel_entry = riscv_load_kernel(machine->kernel_filename,
> > >> > + kernel_start_addr,
> > >> > htif_symbol_callback);
> > >> >
> > >> > if (machine->initrd_filename) { diff --git
> > >> > a/hw/riscv/virt.c b/hw/riscv/virt.c index 41bd2f38ba..6bfd10dfc7
> > >> > 100644
> > >> > --- a/hw/riscv/virt.c
> > >> > +++ b/hw/riscv/virt.c
> > >> > @@ -493,6 +493,7 @@ static void virt_machine_init(MachineState
> > *machine)
> > >> > char *plic_hart_config, *soc_name;
> > >> > size_t plic_hart_config_len;
> > >> > target_ulong start_addr = memmap[VIRT_DRAM].base;
> > >> > + target_ulong firmware_end_addr, kernel_start_addr;
> > >> > uint32_t fdt_load_addr;
> > >> > uint64_t kernel_entry;
> > >> > DeviceState *mmio_plic, *virtio_plic, *pcie_plic; @@ -602,11
> > >> > +603,15 @@ static void virt_machine_init(MachineState *machine)
> > >> > memory_region_add_subregion(system_memory,
> > memmap[VIRT_MROM].base,
> > >> > mask_rom);
> > >> >
> > >> > - riscv_find_and_load_firmware(machine, BIOS_FILENAME,
> > >> > - memmap[VIRT_DRAM].base, NULL);
> > >> > + firmware_end_addr = riscv_find_and_load_firmware(machine,
> > BIOS_FILENAME,
> > >> > + start_addr,
> > >> > + NULL);
> > >> >
> > >> > if (machine->kernel_filename) {
> > >> > - kernel_entry = riscv_load_kernel(machine->kernel_filename,
> > NULL);
> > >> > + kernel_start_addr = riscv_calc_kernel_start_addr(machine,
> > >> > +
> > >> > + firmware_end_addr);
> > >> > +
> > >> > + kernel_entry = riscv_load_kernel(machine->kernel_filename,
> > >> > + kernel_start_addr, NULL);
> > >> >
> > >> > if (machine->initrd_filename) {
> > >> > hwaddr start;
> > >>
> > >> It's probably worth going through and making sure we mark the right
> > >> regions as reserved in the DT, as with these being mobile we might run
> > into latent bugs.
> > >
> > > Do you mean mark where the firmware is as reserved?
> >
> > Ya. We have some things like Linux assuming that the first page in memory is
> > reserved for the bootloader, which IIRC never really got written down in any
> > specifications. That's an implicit S-mode ABI, so it doesn't apply directly, but
> > I'd guess there's some of this floating around elsewhere in the stack.
>
> Let's not do any DT reservation based on firmware load location because
> firmwares can relocate itself to some other address. Further, firmware such
> as OpenSBI can now create domains where only firmware knows how the
> system is partitioned into domains and do DT reservations accordingly.
>
> I suggest we let firmware add the required reservations in DT
Agreed. I don't think QEMU should be editing a DT for the firmware.
The firmware should do that itself if it wants to or needs to.
Alistair
>
> >
> > >> I haven't actually looked so maybe we're fine, but IIRC we sort of
> > >> papered over a handful of memory layout agreements that weren't even
> > >> in specs (or event meant to be in specs) that have stuck around for quite a
> > while.
> > >
> > > For the virt machine or the sifive_u?
> >
> > My guess is that any of this would apply to both of these, as the issue would
> > be assumptions within the firmware about the memory image provided to it
> > when it's entered. That would be an ABI between the firmware and
> > whatever loads it, but IIRC for both the virt board and the sifive_u we
> > assume the firmware is just loaded directly my QEMU.
> >
> > The sifive_u board may be additionally constrained by SiFive's boot ROM, but
> > I don't remember it really caring about any of this. Also not sure it's even
> > used any more, as it was really just a vehicle to demonstrate initializing the
> > widgets for which we couldn't release RTL.
> >
> > >> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
> > >
> > > Thanks
> > >
> > > Alistair
>
> Regards,
> Anup
>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-11-09 23:33 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-14 0:17 [PATCH v2 0/4] Allow loading a no MMU kernel Alistair Francis
2020-10-14 0:17 ` [PATCH v2 1/4] hw/riscv: sifive_u: Allow specifying the CPU Alistair Francis
2020-10-19 23:17 ` Palmer Dabbelt
2020-10-20 3:17 ` Bin Meng
2020-10-14 0:17 ` [PATCH v2 2/4] hw/riscv: Return the end address of the loaded firmware Alistair Francis
2020-10-19 23:17 ` Palmer Dabbelt
2020-10-20 3:17 ` Bin Meng
2020-10-14 0:17 ` [PATCH v2 3/4] hw/riscv: Add a riscv_is_32_bit() function Alistair Francis
2020-10-19 23:17 ` Palmer Dabbelt
2020-10-20 3:17 ` Bin Meng
2020-10-14 0:17 ` [PATCH v2 4/4] hw/riscv: Load the kernel after the firmware Alistair Francis
2020-10-19 23:17 ` Palmer Dabbelt
2020-10-20 15:46 ` Alistair Francis
2020-11-06 2:48 ` Palmer Dabbelt
2020-11-06 4:15 ` Anup Patel
2020-11-09 23:19 ` Alistair Francis
2020-10-20 3:17 ` Bin Meng
2020-10-20 15:44 ` [PATCH v2 0/4] Allow loading a no MMU kernel Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).