Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Daniel Henrique Barboza]

On 1/13/23 07:30, Bin Meng wrote:

On Fri, Jan 13, 2023 at 6:23 PM Daniel Henrique Barboza
<dbarb...@ventanamicro.com> wrote:

On 1/13/23 04:16, Philippe Mathieu-Daudé wrote:

On 12/1/23 01:34, Alistair Francis wrote:

On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
<dbarb...@ventanamicro.com> wrote:

The microchip_icicle_kit, sifive_u, spike and virt boards are now doing
the same steps when '-kernel' is used:

- execute load_kernel()
- load init_rd()
- write kernel_cmdline

Let's fold everything inside riscv_load_kernel() to avoid code
repetition. To not change the behavior of boards that aren't calling
riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and
allow these boards to opt out from initrd loading.

Cc: Palmer Dabbelt <pal...@dabbelt.com>
Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
---
   hw/riscv/boot.c            | 22 +++++++++++++++++++---
   hw/riscv/microchip_pfsoc.c | 12 ++----------
   hw/riscv/opentitan.c       |  2 +-
   hw/riscv/sifive_e.c        |  3 ++-
   hw/riscv/sifive_u.c        | 12 ++----------
   hw/riscv/spike.c           | 11 +----------
   hw/riscv/virt.c            | 12 ++----------
   include/hw/riscv/boot.h    |  1 +
   8 files changed, 30 insertions(+), 45 deletions(-)
@@ -192,21 +194,35 @@ 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;

This breaks 32-bit Xvisor loading. It seems that for the 32-bit guest
we get a value of 0xffffffff80000000.

Previously the top bits would be lost as we return a target_ulong from
this function, but with this change we pass the value
0xffffffff80000000 to riscv_load_initrd() which causes failures.

This diff fixes the failure for me

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 4888d5c1e0..f08ed44b97 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -194,7 +194,7 @@ 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) {
-        kernel_entry = kernel_load_base;
+        kernel_entry = (target_ulong) kernel_load_base;
          goto out;
      }


but I don't think that's the right fix. We should instead look at the
CPU XLEN and drop the high bits if required.

Ah, that is what should be done in load_elf_ram_sym()'s missing
translate_fn() handler.

Interesting. I'll try it again and re-send.

If that fixes the problem, it should be a separate patch.

Fair enough. I'll keep this patch as is and fix it in a separated patch.

Daniel

I still don't understand why 32-bit xvisor image has a 64-bit address encoded?

Regards,
Bin