Re: [PATCH v2 3/6] target/riscv/tcg: add tcg_cpu_finalize_features()

[Alistair Francis]

On Wed, Sep 27, 2023 at 6:05 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The query-cpu-model-expansion API is capable of passing extra properties
> to a given CPU model and tell callers if this custom configuration is
> valid.
>
> The RISC-V version of the API is not quite there yet. The reason is the
> realize() flow in the TCG driver, where most of the validation is done
> in tcg_cpu_realizefn(). riscv_cpu_finalize_features() is then used to
> validate satp_mode for both TCG and KVM CPUs.
>
> Our ARM friends uses a concept of 'finalize_features()', a step done in
> the end of realize() where the CPU features are validated. We have a
> riscv_cpu_finalize_features() helper that, at this moment, is only
> validating satp_mode.
>
> Re-use this existing helper to do all CPU extension validation we
> required after at the end of realize(). Make it public to allow APIs to
> use it. At this moment only the TCG driver requires a realize() time
> validation, thus, to avoid adding accelerator specific helpers in the
> API, riscv_cpu_finalize_features() uses
> riscv_tcg_cpu_finalize_features() if we are running TCG. The API will
> then use riscv_cpu_finalize_features() regardless of the current
> accelerator.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 18 +++++++++--
>  target/riscv/cpu.h         |  1 +
>  target/riscv/tcg/tcg-cpu.c | 61 +++++++++++++++++++++-----------------
>  target/riscv/tcg/tcg-cpu.h |  1 +
>  4 files changed, 51 insertions(+), 30 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 521bb88538..272baaf6c7 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -34,6 +34,7 @@
>  #include "sysemu/kvm.h"
>  #include "sysemu/tcg.h"
>  #include "kvm/kvm_riscv.h"
> +#include "tcg/tcg-cpu.h"
>  #include "tcg/tcg.h"
>
>  /* RISC-V CPU definitions */
> @@ -996,11 +997,24 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, 
> Error **errp)
>  }
>  #endif
>
> -static void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
> +void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
>  {
> -#ifndef CONFIG_USER_ONLY
>      Error *local_err = NULL;
>
> +    /*
> +     * KVM accel does not have a specialized finalize()
> +     * callback because its extensions are validated
> +     * in the get()/set() callbacks of each property.
> +     */
> +    if (tcg_enabled()) {
> +        riscv_tcg_cpu_finalize_features(cpu, &local_err);
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +
> +#ifndef CONFIG_USER_ONLY
>      riscv_cpu_satp_mode_finalize(cpu, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 3f11e69223..1bfa3da55b 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -732,6 +732,7 @@ typedef struct isa_ext_data {
>  extern const RISCVIsaExtData isa_edata_arr[];
>  char *riscv_cpu_get_name(RISCVCPU *cpu);
>
> +void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp);
>  void riscv_add_satp_mode_properties(Object *obj);
>
>  /* CSR function table */
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index a90ee63b06..52cd87db0c 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -549,6 +549,39 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> Error **errp)
>      riscv_cpu_disable_priv_spec_isa_exts(cpu);
>  }
>
> +void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
> +{
> +    CPURISCVState *env = &cpu->env;
> +    Error *local_err = NULL;
> +
> +    riscv_cpu_validate_priv_spec(cpu, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    riscv_cpu_validate_misa_priv(env, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    if (cpu->cfg.epmp && !cpu->cfg.pmp) {
> +        /*
> +         * Enhanced PMP should only be available
> +         * on harts with PMP support
> +         */
> +        error_setg(errp, "Invalid configuration: EPMP requires PMP support");
> +        return;
> +    }
> +
> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +}
> +
>  static bool riscv_cpu_is_generic(Object *cpu_obj)
>  {
>      return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL;
> @@ -564,7 +597,6 @@ static bool riscv_cpu_is_generic(Object *cpu_obj)
>  static bool tcg_cpu_realizefn(CPUState *cs, Error **errp)
>  {
>      RISCVCPU *cpu = RISCV_CPU(cs);
> -    CPURISCVState *env = &cpu->env;
>      Error *local_err = NULL;
>
>      if (object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_CPU_HOST)) {
> @@ -580,33 +612,6 @@ static bool tcg_cpu_realizefn(CPUState *cs, Error **errp)
>          return false;
>      }
>
> -    riscv_cpu_validate_priv_spec(cpu, &local_err);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        return false;
> -    }
> -
> -    riscv_cpu_validate_misa_priv(env, &local_err);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        return false;
> -    }
> -
> -    if (cpu->cfg.epmp && !cpu->cfg.pmp) {
> -        /*
> -         * Enhanced PMP should only be available
> -         * on harts with PMP support
> -         */
> -        error_setg(errp, "Invalid configuration: EPMP requires PMP support");
> -        return false;
> -    }
> -
> -    riscv_cpu_validate_set_extensions(cpu, &local_err);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        return false;
> -    }
> -
>  #ifndef CONFIG_USER_ONLY
>      CPU(cs)->tcg_cflags |= CF_PCREL;
>
> diff --git a/target/riscv/tcg/tcg-cpu.h b/target/riscv/tcg/tcg-cpu.h
> index 630184759d..aa00fbc253 100644
> --- a/target/riscv/tcg/tcg-cpu.h
> +++ b/target/riscv/tcg/tcg-cpu.h
> @@ -23,5 +23,6 @@
>  #include "cpu.h"
>
>  void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
> +void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp);
>
>  #endif
> --
> 2.41.0
>
>