Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities

[Andrew Jones <ajones@ventanamicro.com>]

On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote:
> Currently, the max satp mode is set with the only constraint that it must be
> implemented in qemu, i.e. set in valid_vm_1_10_[32|64].
> 
> But we actually need to add another level of constraint: what the hw is
> actually capable of, because currently, a linux booting on a sifive-u54
> boots in sv57 mode which is incompatible with the cpu's sv39 max
> capability.
> 
> So add a new bitmap to RISCVSATPMap which contains this capability and
> initialize it in every XXX_cpu_init.
> 
> Finally, we have the following chain of constraints:
> 
> Qemu capability > HW capability > User choice > Software capability

                                                  ^ What software is this?
                         I'd think the user's choice would always be last.

> 
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++---------------
>  target/riscv/cpu.h |  8 +++--
>  2 files changed, 59 insertions(+), 27 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index e409e6ab64..19a37fee2b 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
>      g_assert_not_reached();
>  }
>  
> -/* Sets the satp mode to the max supported */
> -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> +                                        const char *satp_mode_str,
> +                                        bool is_32_bit)
>  {
> -    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> -        cpu->cfg.satp_mode.map |=
> -                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> -    } else {
> -        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> +    uint8_t satp_mode = satp_mode_from_str(satp_mode_str);
> +    const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> +
> +    for (int i = 0; i <= satp_mode; ++i) {
> +        if (valid_vm[i]) {
> +            cpu->cfg.satp_mode.supported |= (1 << i);

I don't think we need a new 'supported' bitmap, I think each board that
needs to further constrain va-bits from what QEMU supports should just set
valid_vm_1_10_32/64. I.e. drop const from the arrays and add an init
function something like

 #define QEMU_SATP_MODE_MAX VM_1_10_SV64

 void riscv_cpu_set_satp_mode_max(RISCVCPU *cpu, uint8_t satp_mode_max)
 {
     bool is_32_bit = cpu->env.misa_mxl == MXL_RV32;
     bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;

     g_assert(satp_mode_max <= QEMU_SATP_MODE_MAX);
     g_assert(!is_32_bit || satp_mode_max < 2);

     memset(valid_vm, 0, sizeof(*valid_vm));

     for (int i = 0; i <= satp_mode_max; i++) {
         valid_vm[i] = true;
     }
 }

The valid_vm[] checks already in finalize should then manage the
validation needed to constrain boards. Only boards that care about
this need to call this function, otherwise they'll get the default.

Also, this patch should come before the patch that changes the default
for all boards to sv57 in order to avoid breaking bisection.

Thanks,
drew