Re: [PATCH 02/16] hw/riscv/virt.c: skip 'mmu-type' FDT if satp mode not set

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:09PM -0300, Daniel Henrique Barboza wrote:
> The absence of a satp mode in riscv_host_cpu_init() is causing the
> following error:
> 
> $ sudo ./qemu/build/qemu-system-riscv64  -machine virt,accel=kvm \
>     -m 2G -smp 1  -nographic -snapshot \
>     -kernel ./guest_imgs/Image \
>     -initrd ./guest_imgs/rootfs_kvm_riscv64.img \
>     -append "earlycon=sbi root=/dev/ram rw" \
>     -cpu host
> **
> ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should not be
> reached
> Bail out! ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should
> not be reached
> Aborted
> 
> The error is triggered from create_fdt_socket_cpus() in hw/riscv/virt.c.
> It's trying to get satp_mode_str for a NULL cpu->cfg.satp_mode.map.
> 
> For this KVM 'cpu' we would need to inherit the satp supported modes
> from the RISC-V host. At this moment this is not possible because the
> KVM driver does not support it. And even when it does we can't just let
> this broken for every other older kernel.
> 
> Skip the 'mmu-type' FDT node if there's no satp_mode set.

This seems reasonable, since mmu-type is not a required node, according to
[1], and there's nothing we can put in it, which would be correct, until
we can get the information from KVM.

[1] 
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/cpu.yaml

> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/virt.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 4e3efbee16..8aa907e81f 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -243,13 +243,13 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, 
> int socket,
>              s->soc[socket].hartid_base + cpu);
>          qemu_fdt_add_subnode(ms->fdt, cpu_name);
>  
> -        satp_mode_max = satp_mode_max_from_map(
> -            s->soc[socket].harts[cpu].cfg.satp_mode.map);
> -        sv_name = g_strdup_printf("riscv,%s",
> -                                  satp_mode_str(satp_mode_max, is_32_bit));
> -        qemu_fdt_setprop_string(ms->fdt, cpu_name, "mmu-type", sv_name);
> -        g_free(sv_name);
> -
> +        if (cpu_ptr->cfg.satp_mode.supported != 0) {
> +            satp_mode_max = 
> satp_mode_max_from_map(cpu_ptr->cfg.satp_mode.map);
> +            sv_name = g_strdup_printf("riscv,%s",
> +                                      satp_mode_str(satp_mode_max, 
> is_32_bit));
> +            qemu_fdt_setprop_string(ms->fdt, cpu_name, "mmu-type", sv_name);
> +            g_free(sv_name);
> +        }
>  
>          name = riscv_isa_string(cpu_ptr);
>          qemu_fdt_setprop_string(ms->fdt, cpu_name, "riscv,isa", name);
> -- 
> 2.40.1
> 
>

Adding a sentence, like what I wrote above, to the commit message in order
to provide better justification might be nice, but either way

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew