Re: [PATCH 1/6] target/riscv: Add fw_dynamic_info32 for booting RV32 OpenSBI

[Alistair Francis]

On Mon, Jul 1, 2024 at 1:40 PM LIU Zhiwei <zhiwei_liu@linux.alibaba.com> wrote:
>
> From: TANG Tiancheng <tangtiancheng.ttc@alibaba-inc.com>
>
> RV32 OpenSBI need a fw_dynamic_info parameter with 32-bit fields instead
> of target_ulong.
>
> In RV64 QEMU, target_ulong is 64. So it is not right for booting RV32 OpenSBI.
> We create a fw_dynmaic_info32 struct for this purpose.
>
> Signed-off-by: TANG Tiancheng <tangtiancheng.ttc@alibaba-inc.com>
> Reviewed-by: Liu Zhiwei <zhiwei_liu@linux.alibaba.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/boot.c                 | 35 ++++++++++++++++++++++-----------
>  hw/riscv/sifive_u.c             |  3 ++-
>  include/hw/riscv/boot.h         |  4 +++-
>  include/hw/riscv/boot_opensbi.h | 29 +++++++++++++++++++++++++++
>  4 files changed, 57 insertions(+), 14 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 47281ca853..1a2c1ff9e0 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -342,27 +342,33 @@ void riscv_load_fdt(hwaddr fdt_addr, void *fdt)
>                          rom_ptr_for_as(&address_space_memory, fdt_addr, 
> fdtsize));
>  }
>
> -void riscv_rom_copy_firmware_info(MachineState *machine, hwaddr rom_base,
> -                                  hwaddr rom_size, uint32_t reset_vec_size,
> +void riscv_rom_copy_firmware_info(MachineState *machine,
> +                                  RISCVHartArrayState *harts,
> +                                  hwaddr rom_base, hwaddr rom_size,
> +                                  uint32_t reset_vec_size,
>                                    uint64_t kernel_entry)
>  {
> +    struct fw_dynamic_info32 dinfo32;
>      struct fw_dynamic_info dinfo;
>      size_t dinfo_len;
>
> -    if (sizeof(dinfo.magic) == 4) {
> -        dinfo.magic = cpu_to_le32(FW_DYNAMIC_INFO_MAGIC_VALUE);
> -        dinfo.version = cpu_to_le32(FW_DYNAMIC_INFO_VERSION);
> -        dinfo.next_mode = cpu_to_le32(FW_DYNAMIC_INFO_NEXT_MODE_S);
> -        dinfo.next_addr = cpu_to_le32(kernel_entry);
> +    if (riscv_is_32bit(harts)) {
> +        dinfo32.magic = cpu_to_le32(FW_DYNAMIC_INFO_MAGIC_VALUE);
> +        dinfo32.version = cpu_to_le32(FW_DYNAMIC_INFO_VERSION);
> +        dinfo32.next_mode = cpu_to_le32(FW_DYNAMIC_INFO_NEXT_MODE_S);
> +        dinfo32.next_addr = cpu_to_le32(kernel_entry);
> +        dinfo32.options = 0;
> +        dinfo32.boot_hart = 0;
> +        dinfo_len = sizeof(dinfo32);
>      } else {
>          dinfo.magic = cpu_to_le64(FW_DYNAMIC_INFO_MAGIC_VALUE);
>          dinfo.version = cpu_to_le64(FW_DYNAMIC_INFO_VERSION);
>          dinfo.next_mode = cpu_to_le64(FW_DYNAMIC_INFO_NEXT_MODE_S);
>          dinfo.next_addr = cpu_to_le64(kernel_entry);
> +        dinfo.options = 0;
> +        dinfo.boot_hart = 0;
> +        dinfo_len = sizeof(dinfo);
>      }
> -    dinfo.options = 0;
> -    dinfo.boot_hart = 0;
> -    dinfo_len = sizeof(dinfo);
>
>      /**
>       * copy the dynamic firmware info. This information is specific to
> @@ -374,7 +380,10 @@ void riscv_rom_copy_firmware_info(MachineState *machine, 
> hwaddr rom_base,
>          exit(1);
>      }
>
> -    rom_add_blob_fixed_as("mrom.finfo", &dinfo, dinfo_len,
> +    rom_add_blob_fixed_as("mrom.finfo",
> +                           riscv_is_32bit(harts) ?
> +                           (void *)&dinfo32 : (void *)&dinfo,
> +                           dinfo_len,
>                             rom_base + reset_vec_size,
>                             &address_space_memory);
>  }
> @@ -430,7 +439,9 @@ void riscv_setup_rom_reset_vec(MachineState *machine, 
> RISCVHartArrayState *harts
>      }
>      rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
>                            rom_base, &address_space_memory);
> -    riscv_rom_copy_firmware_info(machine, rom_base, rom_size, 
> sizeof(reset_vec),
> +    riscv_rom_copy_firmware_info(machine, harts,
> +                                 rom_base, rom_size,
> +                                 sizeof(reset_vec),
>                                   kernel_entry);
>  }
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index af5f923f54..5010c3eadb 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -646,7 +646,8 @@ static void sifive_u_machine_init(MachineState *machine)
>      rom_add_blob_fixed_as("mrom.reset", reset_vec, sizeof(reset_vec),
>                            memmap[SIFIVE_U_DEV_MROM].base, 
> &address_space_memory);
>
> -    riscv_rom_copy_firmware_info(machine, memmap[SIFIVE_U_DEV_MROM].base,
> +    riscv_rom_copy_firmware_info(machine, &s->soc.u_cpus,
> +                                 memmap[SIFIVE_U_DEV_MROM].base,
>                                   memmap[SIFIVE_U_DEV_MROM].size,
>                                   sizeof(reset_vec), kernel_entry);
>
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index a2e4ae9cb0..806256d23f 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -56,7 +56,9 @@ void riscv_setup_rom_reset_vec(MachineState *machine, 
> RISCVHartArrayState *harts
>                                 hwaddr rom_base, hwaddr rom_size,
>                                 uint64_t kernel_entry,
>                                 uint64_t fdt_load_addr);
> -void riscv_rom_copy_firmware_info(MachineState *machine, hwaddr rom_base,
> +void riscv_rom_copy_firmware_info(MachineState *machine,
> +                                  RISCVHartArrayState *harts,
> +                                  hwaddr rom_base,
>                                    hwaddr rom_size,
>                                    uint32_t reset_vec_size,
>                                    uint64_t kernel_entry);
> diff --git a/include/hw/riscv/boot_opensbi.h b/include/hw/riscv/boot_opensbi.h
> index 1b749663dc..18664a174b 100644
> --- a/include/hw/riscv/boot_opensbi.h
> +++ b/include/hw/riscv/boot_opensbi.h
> @@ -58,4 +58,33 @@ struct fw_dynamic_info {
>      target_long boot_hart;
>  };
>
> +/** Representation dynamic info passed by previous booting stage */
> +struct fw_dynamic_info32 {
> +    /** Info magic */
> +    int32_t magic;
> +    /** Info version */
> +    int32_t version;
> +    /** Next booting stage address */
> +    int32_t next_addr;
> +    /** Next booting stage mode */
> +    int32_t next_mode;
> +    /** Options for OpenSBI library */
> +    int32_t options;
> +    /**
> +     * Preferred boot HART id
> +     *
> +     * It is possible that the previous booting stage uses same link
> +     * address as the FW_DYNAMIC firmware. In this case, the relocation
> +     * lottery mechanism can potentially overwrite the previous booting
> +     * stage while other HARTs are still running in the previous booting
> +     * stage leading to boot-time crash. To avoid this boot-time crash,
> +     * the previous booting stage can specify last HART that will jump
> +     * to the FW_DYNAMIC firmware as the preferred boot HART.
> +     *
> +     * To avoid specifying a preferred boot HART, the previous booting
> +     * stage can set it to -1UL which will force the FW_DYNAMIC firmware
> +     * to use the relocation lottery mechanism.
> +     */
> +    int32_t boot_hart;
> +};
>  #endif
> --
> 2.43.0
>
>