Re: [PATCH v8 04/19] target/riscv/cpu.c: set satp_mode_max MBARE during satp_finalize()

[Andrew Jones]

On Wed, Nov 01, 2023 at 05:41:49PM -0300, Daniel Henrique Barboza wrote:
> KVM CPUs can handle "cpu->cfg.satp_mode.supported == 0" because KVM will
> make it do internally, not requiring the current SATP support from TCG.
> 
> But other TCG CPUs doesn't deal well with it. We'll assert out before
> OpenSBI if the CPU doesn't set a default:
> 
> ERROR:../target/riscv/cpu.c:317:satp_mode_max_from_map: assertion failed: 
> (map > 0)
> Bail out! ERROR:../target/riscv/cpu.c:317:satp_mode_max_from_map: assertion 
> failed: (map > 0)
> 
> This will be thrown by target/riscv/csr.c, write_satp(), when stepping
> in validate_vm().
> 
> There's no current CPUs affected by it, but next patch will add a new
> CPU that doesn't have defaults and this assert will be hit.
> 
> Change riscv_cpu_satp_mode_finalize() to set satp_mode_max_supported()
> to MBARE if the CPU happens to not have a max mode set yet.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 9f6837ecb7..f7c1989d14 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -942,9 +942,19 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, 
> Error **errp)
>      bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
>      uint8_t satp_mode_map_max, satp_mode_supported_max;
>  
> -    /* The CPU wants the OS to decide which satp mode to use */
>      if (cpu->cfg.satp_mode.supported == 0) {
> -        return;
> +        if (kvm_enabled()) {
> +            /* The CPU wants the OS to decide which satp mode to use */
> +            return;
> +        }
> +
> +        /*
> +         * We do not handle cpu->cfg.satp_mode.supported == 0
> +         * with TCG yet. Set to MBARE.
> +         */
> +        if (tcg_enabled()) {
> +            set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
> +        }
>      }
>  
>      satp_mode_supported_max =
> -- 
> 2.41.0
>

This patch should no longer be necessary if the suggestion I made in the
previous patch works the way I think it should. But, regarding this patch,
I do like that the "The CPU wants the OS to decide which satp mode to use"
comment became specific to KVM, which makes more sense to me. Maybe we
should just change that comment to something like

/*
 * With some accelerators, such as KVM, the OS dictates which satp mode to
 * use. For those cases, satp_mode.supported is zero and there's nothing
 * to do here.
 */

Thanks,
drew