From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
To: Bin Meng <bmeng@tinylab.org>,
Alistair Francis <Alistair.Francis@wdc.com>,
qemu-devel@nongnu.org
Cc: "Bin Meng" <bin.meng@windriver.com>,
"Marc-André Lureau" <marcandre.lureau@redhat.com>,
"Palmer Dabbelt" <palmer@dabbelt.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
qemu-riscv@nongnu.org
Subject: Re: [PATCH 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading
Date: Tue, 27 Dec 2022 11:33:12 -0300 [thread overview]
Message-ID: <d501aa56-31c1-0efb-009c-bc0a991397eb@ventanamicro.com> (raw)
In-Reply-To: <20221227064812.1903326-13-bmeng@tinylab.org>
Hi Bin,
On 12/27/22 03:48, Bin Meng wrote:
> At present create_fdt() calls htif_uses_elf_symbols() to determine
> whether to insert a <reg> property for the HTIF. This unfortunately
> creates a hidden dependency to riscv_load_{firmware,kernel} that
> create_fdt() must be called after the ELF {firmware,kernel} image
> has been loaded.
>
> Decouple such dependency be adding a new parameter to create_fdt(),
> whether custom HTIF base address is used. The flag will be set if
> non ELF {firmware,kernel} image is given by user.
>
> Signed-off-by: Bin Meng <bmeng@tinylab.org>
>
> ---
>
> include/hw/char/riscv_htif.h | 5 +---
> hw/char/riscv_htif.c | 17 +++++-------
> hw/riscv/spike.c | 54 ++++++++++++++++++++++++++++++------
> 3 files changed, 53 insertions(+), 23 deletions(-)
>
> diff --git a/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
> index 9e8ebbe017..5958c5b986 100644
> --- a/include/hw/char/riscv_htif.h
> +++ b/include/hw/char/riscv_htif.h
> @@ -44,11 +44,8 @@ typedef struct HTIFState {
> void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
> uint64_t st_size);
>
> -/* Check if HTIF uses ELF symbols */
> -bool htif_uses_elf_symbols(void);
> -
> /* legacy pre qom */
> HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
> - uint64_t nonelf_base);
> + uint64_t nonelf_base, bool custom_base);
>
> #endif
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 1477fc0090..098de50e35 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -52,20 +52,17 @@
> #define PK_SYS_WRITE 64
>
> static uint64_t fromhost_addr, tohost_addr;
> -static int address_symbol_set;
>
> void htif_symbol_callback(const char *st_name, int st_info, uint64_t st_value,
> uint64_t st_size)
> {
> if (strcmp("fromhost", st_name) == 0) {
> - address_symbol_set |= 1;
> fromhost_addr = st_value;
> if (st_size != 8) {
> error_report("HTIF fromhost must be 8 bytes");
> exit(1);
> }
> } else if (strcmp("tohost", st_name) == 0) {
> - address_symbol_set |= 2;
> tohost_addr = st_value;
> if (st_size != 8) {
> error_report("HTIF tohost must be 8 bytes");
> @@ -275,19 +272,19 @@ static const MemoryRegionOps htif_mm_ops = {
> .write = htif_mm_write,
> };
>
> -bool htif_uses_elf_symbols(void)
> -{
> - return (address_symbol_set == 3) ? true : false;
> -}
> -
> HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
> - uint64_t nonelf_base)
> + uint64_t nonelf_base, bool custom_base)
> {
> uint64_t base, size, tohost_offset, fromhost_offset;
>
> - if (!htif_uses_elf_symbols()) {
> + if (custom_base) {
> fromhost_addr = nonelf_base;
> tohost_addr = nonelf_base + 8;
> + } else {
> + if (!fromhost_addr || !tohost_addr) {
> + error_report("Invalid HTIF fromhost or tohost address");
> + exit(1);
> + }
> }
>
> base = MIN(tohost_addr, fromhost_addr);
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 810a18f283..90f9e581e4 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -50,7 +50,8 @@ static const MemMapEntry spike_memmap[] = {
> };
>
> static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
> - uint64_t mem_size, const char *cmdline, bool is_32_bit)
> + uint64_t mem_size, const char *cmdline,
> + bool is_32_bit, bool htif_custom_base)
> {
> void *fdt;
> uint64_t addr, size;
> @@ -78,7 +79,7 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
>
> qemu_fdt_add_subnode(fdt, "/htif");
> qemu_fdt_setprop_string(fdt, "/htif", "compatible", "ucb,htif0");
> - if (!htif_uses_elf_symbols()) {
> + if (htif_custom_base) {
> qemu_fdt_setprop_cells(fdt, "/htif", "reg",
> 0x0, memmap[SPIKE_HTIF].base, 0x0, memmap[SPIKE_HTIF].size);
> }
> @@ -184,6 +185,21 @@ static void create_fdt(SpikeState *s, const MemMapEntry *memmap,
> }
> }
>
> +static bool spike_test_elf_image(char *filename)
> +{
> + Error *err = NULL;
> +
> + if (filename) {
> + load_elf_hdr(filename, NULL, NULL, &err);
> + if (err) {
> + error_free(err);
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> static void spike_board_init(MachineState *machine)
> {
> const MemMapEntry *memmap = spike_memmap;
> @@ -191,11 +207,12 @@ static void spike_board_init(MachineState *machine)
> MemoryRegion *system_memory = get_system_memory();
> MemoryRegion *mask_rom = g_new(MemoryRegion, 1);
> target_ulong firmware_end_addr, kernel_start_addr;
> - const char *firmware_name;
> + char *firmware_name;
> uint32_t fdt_load_addr;
> uint64_t kernel_entry;
> char *soc_name;
> int i, base_hartid, hart_count;
> + bool htif_custom_base;
>
> /* Check socket count limit */
> if (SPIKE_SOCKETS_MAX < riscv_socket_count(machine)) {
> @@ -257,10 +274,28 @@ static void spike_board_init(MachineState *machine)
> memory_region_add_subregion(system_memory, memmap[SPIKE_MROM].base,
> mask_rom);
>
> - firmware_name = riscv_default_firmware_name(&s->soc[0]);
> - firmware_end_addr = riscv_find_and_load_firmware(machine, firmware_name,
> - memmap[SPIKE_DRAM].base,
> - htif_symbol_callback);
> + /* Find firmware */
> + firmware_name = riscv_find_firmware(machine->firmware,
> + riscv_default_firmware_name(&s->soc[0]));
> +
> + /*
> + * Test the given firmware or kernel file to see if it is an ELF image.
> + * If it is an ELF, we assume it contains the symbols required for
> + * the HTIF console, otherwise we fall back to use the custom base
> + * passed from device tree for the HTIF console.
> + */
> + htif_custom_base = !spike_test_elf_image(firmware_name);
> + if (!htif_custom_base) {
> + htif_custom_base = !spike_test_elf_image(machine->kernel_filename);
> + }
> +
> + /* Load firmware */
> + if (firmware_name) {
> + firmware_end_addr = riscv_load_firmware(firmware_name,
> + memmap[SPIKE_DRAM].base,
> + htif_symbol_callback);
> + g_free(firmware_name);
> + }
>
> /* Load kernel */
> if (machine->kernel_filename) {
'make' is giving me a maybe-uninitialized error in this point:
../hw/riscv/spike.c: In function ‘spike_board_init’:
../hw/riscv/spike.c:301:29: error: ‘firmware_end_addr’ may be used uninitialized [-Werror=maybe-uninitialized]
301 | kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
302 | firmware_end_addr);
| ~~~~~~~~~~~~~~~~~~
../hw/riscv/spike.c:208:18: note: ‘firmware_end_addr’ was declared here
208 | target_ulong firmware_end_addr, kernel_start_addr;
| ^~~~~~~~~~~~~~~~~
The full context:
/* Load firmware */
if (firmware_name) {
firmware_end_addr = riscv_load_firmware(firmware_name,
memmap[SPIKE_DRAM].base,
htif_symbol_callback);
g_free(firmware_name);
}
/* Load kernel */
if (machine->kernel_filename) {
kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],
firmware_end_addr);
kernel_entry = riscv_load_kernel(machine->kernel_filename,
kernel_start_addr,
htif_symbol_callback);
} else {
The error is happening because, with this patch, 'firmware_end_addr' may not be
initialized via riscv_load_firmware() because we're not guaranteeing that
'firmware_name' will be valid.
riscv_load_firmware() is guaranteed to either return > 0 or error out with exit(1),
so a simple fix would be to initialize 'firmware_end_addr' with 0.
Thanks,
Daniel
> @@ -280,7 +315,7 @@ static void spike_board_init(MachineState *machine)
>
> /* Create device tree */
> create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline,
> - riscv_is_32bit(&s->soc[0]));
> + riscv_is_32bit(&s->soc[0]), htif_custom_base);
>
> /* Load initrd */
> if (machine->kernel_filename && machine->initrd_filename) {
> @@ -308,7 +343,8 @@ static void spike_board_init(MachineState *machine)
> fdt_load_addr);
>
> /* initialize HTIF using symbols found in load_kernel */
> - htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base);
> + htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base,
> + htif_custom_base);
> }
>
> static void spike_machine_instance_init(Object *obj)
next prev parent reply other threads:[~2022-12-27 14:33 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-27 6:48 [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Bin Meng
2022-12-27 6:48 ` [PATCH 01/12] hw/char: riscv_htif: Avoid using magic numbers Bin Meng
2022-12-27 17:28 ` Daniel Henrique Barboza
2022-12-28 3:31 ` Alistair Francis
2022-12-27 6:48 ` [PATCH 02/12] hw/char: riscv_htif: Drop {to, from}host_size in HTIFState Bin Meng
2022-12-27 17:29 ` [PATCH 02/12] hw/char: riscv_htif: Drop {to,from}host_size " Daniel Henrique Barboza
2022-12-28 3:32 ` [PATCH 02/12] hw/char: riscv_htif: Drop {to, from}host_size " Alistair Francis
2022-12-27 6:48 ` [PATCH 03/12] hw/char: riscv_htif: Drop useless assignment of memory region Bin Meng
2022-12-27 17:30 ` Daniel Henrique Barboza
2022-12-28 3:33 ` Alistair Francis
2022-12-27 6:48 ` [PATCH 04/12] hw/char: riscv_htif: Use conventional 's' for HTIFState Bin Meng
2022-12-27 17:32 ` Daniel Henrique Barboza
2022-12-28 3:35 ` Alistair Francis
2022-12-27 6:48 ` [PATCH 05/12] hw/char: riscv_htif: Move registers from CPUArchState to HTIFState Bin Meng
2022-12-27 17:33 ` Daniel Henrique Barboza
2022-12-28 4:19 ` Alistair Francis
2022-12-27 6:48 ` [PATCH 06/12] hw/char: riscv_htif: Remove forward declarations for non-existent variables Bin Meng
2022-12-27 17:35 ` Daniel Henrique Barboza
2022-12-28 4:22 ` Alistair Francis
2022-12-27 6:48 ` [PATCH 07/12] hw/char: riscv_htif: Support console output via proxy syscall Bin Meng
2022-12-27 17:37 ` Daniel Henrique Barboza
2022-12-28 4:30 ` Alistair Francis
2022-12-27 6:48 ` [PATCH 08/12] hw/riscv: spike: Remove the out-of-date comments Bin Meng
2022-12-27 17:37 ` Daniel Henrique Barboza
2022-12-28 4:30 ` Alistair Francis
2022-12-27 6:48 ` [PATCH 09/12] hw/riscv/boot.c: make riscv_find_firmware() static Bin Meng
2022-12-27 6:48 ` [PATCH 10/12] hw/riscv/boot.c: introduce riscv_default_firmware_name() Bin Meng
2022-12-27 6:48 ` [PATCH 11/12] hw/riscv/boot.c: Introduce riscv_find_firmware() Bin Meng
2022-12-27 17:40 ` Daniel Henrique Barboza
2022-12-28 4:35 ` Alistair Francis
2022-12-27 6:48 ` [PATCH 12/12] hw/riscv: spike: Decouple create_fdt() dependency to ELF loading Bin Meng
2022-12-27 14:33 ` Daniel Henrique Barboza [this message]
2022-12-27 17:51 ` [PATCH 00/12] hw/riscv: Improve Spike HTIF emulation fidelity Daniel Henrique Barboza
2022-12-28 3:58 ` Bin Meng
2022-12-28 4:21 ` Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d501aa56-31c1-0efb-009c-bc0a991397eb@ventanamicro.com \
--to=dbarboza@ventanamicro.com \
--cc=Alistair.Francis@wdc.com \
--cc=bin.meng@windriver.com \
--cc=bmeng@tinylab.org \
--cc=marcandre.lureau@redhat.com \
--cc=palmer@dabbelt.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.