Re: [PATCH v9 01/23] target/riscv: Move MISA limits to class

[Alex Bennée]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> MISA limits are common for all instances of a RISC-V CPU class so they
> are better put into class.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  target/riscv/cpu-qom.h   |   2 +
>  target/riscv/cpu.h       |   2 -
>  hw/riscv/boot.c          |   2 +-
>  target/riscv/cpu.c       | 212 +++++++++++++++++++++++++++------------
>  target/riscv/csr.c       |   3 +-
>  target/riscv/gdbstub.c   |  12 ++-
>  target/riscv/machine.c   |  11 +-
>  target/riscv/translate.c |   3 +-
>  8 files changed, 167 insertions(+), 80 deletions(-)
>
> diff --git a/target/riscv/cpu-qom.h b/target/riscv/cpu-qom.h
> index 04af50983e..266a07f5be 100644
> --- a/target/riscv/cpu-qom.h
> +++ b/target/riscv/cpu-qom.h
> @@ -67,5 +67,7 @@ struct RISCVCPUClass {
>      /*< public >*/
>      DeviceRealize parent_realize;
>      ResettablePhases parent_phases;
> +    uint32_t misa_mxl_max;  /* max mxl for this cpu */
> +    uint32_t misa_ext_mask; /* max ext for this cpu */
>  };
>  #endif /* RISCV_CPU_QOM_H */
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index ef9cf21c0c..9f9cb6cd2a 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -155,9 +155,7 @@ struct CPUArchState {
>  
>      /* RISCVMXL, but uint32_t for vmstate migration */
>      uint32_t misa_mxl;      /* current mxl */
> -    uint32_t misa_mxl_max;  /* max mxl for this cpu */
>      uint32_t misa_ext;      /* current extensions */
> -    uint32_t misa_ext_mask; /* max ext for this cpu */
>      uint32_t xl;            /* current xlen */
>  
>      /* 128-bit helpers upper part return value */
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 52bf8e67de..b7cf08f479 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -36,7 +36,7 @@
>  
>  bool riscv_is_32bit(RISCVHartArrayState *harts)
>  {
> -    return harts->harts[0].env.misa_mxl_max == MXL_RV32;
> +    return RISCV_CPU_GET_CLASS(&harts->harts[0])->misa_mxl_max ==
>  MXL_RV32;

I'm going to defer to the RISCV maintainers here. While I agree the
class is a good place for these parameters that are shared across
multiple vCPUS there is a cost to RISCV_CPU_GET_CLASS() casting.

You might notice we have this comment in include/hw/core/cpu.h:

  /*
   * The class checkers bring in CPU_GET_CLASS() which is potentially
   * expensive given the eventual call to
   * object_class_dynamic_cast_assert(). Because of this the CPUState
   * has a cached value for the class in cs->cc which is set up in
   * cpu_exec_realizefn() for use in hot code paths.
   */
  typedef struct CPUClass CPUClass;
  DECLARE_CLASS_CHECKERS(CPUClass, CPU,
                         TYPE_CPU)

However I think you need to check the assumption that you will never see
multiple cores with different RISCV properties.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro