| Message ID | 20230206140022.2748401-2-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Consolidate all kernel init in load_kernel() | expand |
On 6/2/23 15:00, Daniel Henrique Barboza wrote: > Next patch will move all calls to riscv_load_initrd() to > riscv_load_kernel(). Machines that want to load initrd will be able to > do via an extra flag to riscv_load_kernel(). > > This change will expose a sign-extend behavior that is happening in > load_elf_ram_sym() when running 32 bit guests [1]. This is currently > obscured by the fact that riscv_load_initrd() is using the return of > riscv_load_kernel(), defined as target_ulong, and this return type will > crop the higher 32 bits that would be padded with 1s by the sign > extension when running in 32 bit targets. The changes to be done will > force riscv_load_initrd() to use an uint64_t instead, exposing it to the > padding when dealing with 32 bit CPUs. > > There is a discussion about whether load_elf_ram_sym() should or should > not sign extend the value returned by 'lowaddr'. What we can do is to > prevent the behavior change that the next patch will end up doing. > riscv_load_initrd() wasn't dealing with 64 bit kernel entries when > running 32 bit CPUs, and we want to keep it that way. > > One way of doing it is to use target_ulong in 'kernel_entry' in > riscv_load_kernel() and rely on the fact that this var will not be sign > extended for 32 bit targets. Another way is to explictly clear the > higher 32 bits when running 32 bit CPUs for all possibilities of > kernel_entry. > > We opted for the later. This will allow us to be clear about the design > choices made in the function, while also allowing us to add a small > comment about what load_elf_ram_sym() is doing. With this change, the > consolation patch can do its job without worrying about unintended > behavioral changes. > > [1] https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg02281.html > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > hw/riscv/boot.c | 20 +++++++++++++++++--- > hw/riscv/microchip_pfsoc.c | 3 ++- > hw/riscv/opentitan.c | 3 ++- > hw/riscv/sifive_e.c | 3 ++- > hw/riscv/sifive_u.c | 3 ++- > hw/riscv/spike.c | 3 ++- > hw/riscv/virt.c | 3 ++- > include/hw/riscv/boot.h | 1 + > 8 files changed, 30 insertions(+), 9 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Tue, Feb 7, 2023 at 12:04 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > Next patch will move all calls to riscv_load_initrd() to > riscv_load_kernel(). Machines that want to load initrd will be able to > do via an extra flag to riscv_load_kernel(). > > This change will expose a sign-extend behavior that is happening in > load_elf_ram_sym() when running 32 bit guests [1]. This is currently > obscured by the fact that riscv_load_initrd() is using the return of > riscv_load_kernel(), defined as target_ulong, and this return type will > crop the higher 32 bits that would be padded with 1s by the sign > extension when running in 32 bit targets. The changes to be done will > force riscv_load_initrd() to use an uint64_t instead, exposing it to the > padding when dealing with 32 bit CPUs. > > There is a discussion about whether load_elf_ram_sym() should or should > not sign extend the value returned by 'lowaddr'. What we can do is to > prevent the behavior change that the next patch will end up doing. > riscv_load_initrd() wasn't dealing with 64 bit kernel entries when > running 32 bit CPUs, and we want to keep it that way. > > One way of doing it is to use target_ulong in 'kernel_entry' in > riscv_load_kernel() and rely on the fact that this var will not be sign > extended for 32 bit targets. Another way is to explictly clear the > higher 32 bits when running 32 bit CPUs for all possibilities of > kernel_entry. > > We opted for the later. This will allow us to be clear about the design > choices made in the function, while also allowing us to add a small > comment about what load_elf_ram_sym() is doing. With this change, the > consolation patch can do its job without worrying about unintended > behavioral changes. > > [1] https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg02281.html > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > hw/riscv/boot.c | 20 +++++++++++++++++--- > hw/riscv/microchip_pfsoc.c | 3 ++- > hw/riscv/opentitan.c | 3 ++- > hw/riscv/sifive_e.c | 3 ++- > hw/riscv/sifive_u.c | 3 ++- > hw/riscv/spike.c | 3 ++- > hw/riscv/virt.c | 3 ++- > include/hw/riscv/boot.h | 1 + > 8 files changed, 30 insertions(+), 9 deletions(-) > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c > index c7e0e50bd8..df6b4a1fba 100644 > --- a/hw/riscv/boot.c > +++ b/hw/riscv/boot.c > @@ -174,6 +174,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename, > } > > target_ulong riscv_load_kernel(MachineState *machine, > + RISCVHartArrayState *harts, > target_ulong kernel_start_addr, > symbol_fn_t sym_cb) > { > @@ -192,21 +193,34 @@ target_ulong riscv_load_kernel(MachineState *machine, > if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL, > NULL, &kernel_load_base, NULL, NULL, 0, > EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) { > - return kernel_load_base; > + kernel_entry = kernel_load_base; > + goto out; > } > > if (load_uimage_as(kernel_filename, &kernel_entry, NULL, NULL, > NULL, NULL, NULL) > 0) { > - return kernel_entry; > + goto out; > } > > if (load_image_targphys_as(kernel_filename, kernel_start_addr, > current_machine->ram_size, NULL) > 0) { > - return kernel_start_addr; > + kernel_entry = kernel_start_addr; > + goto out; > } > > error_report("could not load kernel '%s'", kernel_filename); > exit(1); > + > +out: > + /* > + * For 32 bit CPUs 'kernel_entry' can be sign-extended by > + * load_elf_ram_sym(). > + */ > + if (riscv_is_32bit(harts)) { > + kernel_entry = extract64(kernel_entry, 0, 32); > + } > + > + return kernel_entry; > } > > void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry) > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c > index 2b91e49561..712625d2a4 100644 > --- a/hw/riscv/microchip_pfsoc.c > +++ b/hw/riscv/microchip_pfsoc.c > @@ -629,7 +629,8 @@ static void microchip_icicle_kit_machine_init(MachineState *machine) > kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus, > firmware_end_addr); > > - kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL); > + kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus, > + kernel_start_addr, NULL); > > if (machine->initrd_filename) { > riscv_load_initrd(machine, kernel_entry); > diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c > index 353f030d80..7fe4fb5628 100644 > --- a/hw/riscv/opentitan.c > +++ b/hw/riscv/opentitan.c > @@ -101,7 +101,8 @@ static void opentitan_board_init(MachineState *machine) > } > > if (machine->kernel_filename) { > - riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, NULL); > + riscv_load_kernel(machine, &s->soc.cpus, > + memmap[IBEX_DEV_RAM].base, NULL); > } > } > > diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c > index 3e3f4b0088..1a7d381514 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, memmap[SIFIVE_E_DEV_DTIM].base, NULL); > + riscv_load_kernel(machine, &s->soc.cpus, > + memmap[SIFIVE_E_DEV_DTIM].base, NULL); > } > } > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > index d3ab7a9cda..71be442a50 100644 > --- a/hw/riscv/sifive_u.c > +++ b/hw/riscv/sifive_u.c > @@ -598,7 +598,8 @@ static void sifive_u_machine_init(MachineState *machine) > kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus, > firmware_end_addr); > > - kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL); > + kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus, > + kernel_start_addr, NULL); > > if (machine->initrd_filename) { > riscv_load_initrd(machine, kernel_entry); > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c > index cc3f6dac17..1fa91167ab 100644 > --- a/hw/riscv/spike.c > +++ b/hw/riscv/spike.c > @@ -305,7 +305,8 @@ static void spike_board_init(MachineState *machine) > kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0], > firmware_end_addr); > > - kernel_entry = riscv_load_kernel(machine, kernel_start_addr, > + kernel_entry = riscv_load_kernel(machine, &s->soc[0], > + kernel_start_addr, > htif_symbol_callback); > > if (machine->initrd_filename) { > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index a061151a6f..d0531cc641 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -1277,7 +1277,8 @@ static void virt_machine_done(Notifier *notifier, void *data) > kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0], > firmware_end_addr); > > - kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL); > + kernel_entry = riscv_load_kernel(machine, &s->soc[0], > + kernel_start_addr, NULL); > > if (machine->initrd_filename) { > riscv_load_initrd(machine, kernel_entry); > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h > index 511390f60e..6295316afb 100644 > --- a/include/hw/riscv/boot.h > +++ b/include/hw/riscv/boot.h > @@ -44,6 +44,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename, > hwaddr firmware_load_addr, > symbol_fn_t sym_cb); > target_ulong riscv_load_kernel(MachineState *machine, > + RISCVHartArrayState *harts, > target_ulong firmware_end_addr, > symbol_fn_t sym_cb); > void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry); > -- > 2.39.1 > >
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c index c7e0e50bd8..df6b4a1fba 100644 --- a/hw/riscv/boot.c +++ b/hw/riscv/boot.c @@ -174,6 +174,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename, } target_ulong riscv_load_kernel(MachineState *machine, + RISCVHartArrayState *harts, target_ulong kernel_start_addr, symbol_fn_t sym_cb) { @@ -192,21 +193,34 @@ target_ulong riscv_load_kernel(MachineState *machine, if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL, NULL, &kernel_load_base, NULL, NULL, 0, EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) { - return kernel_load_base; + kernel_entry = kernel_load_base; + goto out; } if (load_uimage_as(kernel_filename, &kernel_entry, NULL, NULL, NULL, NULL, NULL) > 0) { - return kernel_entry; + goto out; } if (load_image_targphys_as(kernel_filename, kernel_start_addr, current_machine->ram_size, NULL) > 0) { - return kernel_start_addr; + kernel_entry = kernel_start_addr; + goto out; } error_report("could not load kernel '%s'", kernel_filename); exit(1); + +out: + /* + * For 32 bit CPUs 'kernel_entry' can be sign-extended by + * load_elf_ram_sym(). + */ + if (riscv_is_32bit(harts)) { + kernel_entry = extract64(kernel_entry, 0, 32); + } + + return kernel_entry; } void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry) diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c index 2b91e49561..712625d2a4 100644 --- a/hw/riscv/microchip_pfsoc.c +++ b/hw/riscv/microchip_pfsoc.c @@ -629,7 +629,8 @@ static void microchip_icicle_kit_machine_init(MachineState *machine) kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus, firmware_end_addr); - kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL); + kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus, + kernel_start_addr, NULL); if (machine->initrd_filename) { riscv_load_initrd(machine, kernel_entry); diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c index 353f030d80..7fe4fb5628 100644 --- a/hw/riscv/opentitan.c +++ b/hw/riscv/opentitan.c @@ -101,7 +101,8 @@ static void opentitan_board_init(MachineState *machine) } if (machine->kernel_filename) { - riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, NULL); + riscv_load_kernel(machine, &s->soc.cpus, + memmap[IBEX_DEV_RAM].base, NULL); } } diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c index 3e3f4b0088..1a7d381514 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, memmap[SIFIVE_E_DEV_DTIM].base, NULL); + riscv_load_kernel(machine, &s->soc.cpus, + memmap[SIFIVE_E_DEV_DTIM].base, NULL); } } diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index d3ab7a9cda..71be442a50 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -598,7 +598,8 @@ static void sifive_u_machine_init(MachineState *machine) kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus, firmware_end_addr); - kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL); + kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus, + kernel_start_addr, NULL); if (machine->initrd_filename) { riscv_load_initrd(machine, kernel_entry); diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index cc3f6dac17..1fa91167ab 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -305,7 +305,8 @@ static void spike_board_init(MachineState *machine) kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0], firmware_end_addr); - kernel_entry = riscv_load_kernel(machine, kernel_start_addr, + kernel_entry = riscv_load_kernel(machine, &s->soc[0], + kernel_start_addr, htif_symbol_callback); if (machine->initrd_filename) { diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index a061151a6f..d0531cc641 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1277,7 +1277,8 @@ static void virt_machine_done(Notifier *notifier, void *data) kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0], firmware_end_addr); - kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL); + kernel_entry = riscv_load_kernel(machine, &s->soc[0], + kernel_start_addr, NULL); if (machine->initrd_filename) { riscv_load_initrd(machine, kernel_entry); diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h index 511390f60e..6295316afb 100644 --- a/include/hw/riscv/boot.h +++ b/include/hw/riscv/boot.h @@ -44,6 +44,7 @@ target_ulong riscv_load_firmware(const char *firmware_filename, hwaddr firmware_load_addr, symbol_fn_t sym_cb); target_ulong riscv_load_kernel(MachineState *machine, + RISCVHartArrayState *harts, target_ulong firmware_end_addr, symbol_fn_t sym_cb); void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
Next patch will move all calls to riscv_load_initrd() to riscv_load_kernel(). Machines that want to load initrd will be able to do via an extra flag to riscv_load_kernel(). This change will expose a sign-extend behavior that is happening in load_elf_ram_sym() when running 32 bit guests [1]. This is currently obscured by the fact that riscv_load_initrd() is using the return of riscv_load_kernel(), defined as target_ulong, and this return type will crop the higher 32 bits that would be padded with 1s by the sign extension when running in 32 bit targets. The changes to be done will force riscv_load_initrd() to use an uint64_t instead, exposing it to the padding when dealing with 32 bit CPUs. There is a discussion about whether load_elf_ram_sym() should or should not sign extend the value returned by 'lowaddr'. What we can do is to prevent the behavior change that the next patch will end up doing. riscv_load_initrd() wasn't dealing with 64 bit kernel entries when running 32 bit CPUs, and we want to keep it that way. One way of doing it is to use target_ulong in 'kernel_entry' in riscv_load_kernel() and rely on the fact that this var will not be sign extended for 32 bit targets. Another way is to explictly clear the higher 32 bits when running 32 bit CPUs for all possibilities of kernel_entry. We opted for the later. This will allow us to be clear about the design choices made in the function, while also allowing us to add a small comment about what load_elf_ram_sym() is doing. With this change, the consolation patch can do its job without worrying about unintended behavioral changes. [1] https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg02281.html Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- hw/riscv/boot.c | 20 +++++++++++++++++--- hw/riscv/microchip_pfsoc.c | 3 ++- hw/riscv/opentitan.c | 3 ++- hw/riscv/sifive_e.c | 3 ++- hw/riscv/sifive_u.c | 3 ++- hw/riscv/spike.c | 3 ++- hw/riscv/virt.c | 3 ++- include/hw/riscv/boot.h | 1 + 8 files changed, 30 insertions(+), 9 deletions(-)