Re: [PATCH v2 05/18] target/riscv/cpu.c: restrict 'marchid' value

[Alistair Francis]

On Wed, Jun 14, 2023 at 7:03 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 'marchid' shouldn't be set to a different value as previously set for
> named CPUs.
>
> For all other CPUs it shouldn't be freely set either - the spec requires
> that 'marchid' can't have the MSB (most significant bit) set and every
> other bit set to zero, i.e. 0x80000000 is an invalid 'marchid' value for
> 32 bit CPUs.
>
> As with 'mimpid', setting a default value based on the current QEMU
> version is not a good idea because it implies that the CPU
> implementation changes from one QEMU version to the other. Named CPUs
> should set 'marchid' to a meaningful value instead, and generic CPUs can
> set to any valid value.
>
> For the 'veyron-v1' CPU this is the error thrown if 'marchid' is set to
> a different val:
>
> $ ./build/qemu-system-riscv64 -M virt -nographic -cpu 
> veyron-v1,marchid=0x80000000
> qemu-system-riscv64: can't apply global 
> veyron-v1-riscv-cpu.marchid=0x80000000:
>     Unable to change veyron-v1-riscv-cpu marchid (0x8000000000010000)
>
> And, for generics CPUs, this is the error when trying to set to an
> invalid val:
>
> $ ./build/qemu-system-riscv64 -M virt -nographic -cpu 
> rv64,marchid=0x8000000000000000
> qemu-system-riscv64: can't apply global 
> rv64-riscv-cpu.marchid=0x8000000000000000:
>     Unable to set marchid with MSB (64) bit set and the remaining bits zero
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c | 60 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 53 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 39c550682a..2eb793188c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -38,11 +38,6 @@
>  #include "tcg/tcg.h"
>
>  /* RISC-V CPU definitions */
> -
> -#define RISCV_CPU_MARCHID   ((QEMU_VERSION_MAJOR << 16) | \
> -                             (QEMU_VERSION_MINOR << 8)  | \
> -                             (QEMU_VERSION_MICRO))
> -
>  static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
>
>  struct isa_ext_data {
> @@ -1733,8 +1728,6 @@ static void riscv_cpu_add_user_properties(Object *obj)
>  static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
>
> -    DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, RISCV_CPU_MARCHID),
> -
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>  #endif
> @@ -1881,6 +1874,56 @@ static void cpu_get_mimpid(Object *obj, Visitor *v, 
> const char *name,
>      visit_type_bool(v, name, &value, errp);
>  }
>
> +static void cpu_set_marchid(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    uint64_t prev_val = cpu->cfg.marchid;
> +    uint64_t value, invalid_val;
> +    uint32_t mxlen = 0;
> +
> +    if (!visit_type_uint64(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (!dynamic_cpu && prev_val != value) {
> +        error_setg(errp, "Unable to change %s marchid (0x%lx)",
> +                   object_get_typename(obj), prev_val);
> +        return;
> +    }
> +
> +    switch (riscv_cpu_mxl(&cpu->env)) {
> +    case MXL_RV32:
> +        mxlen = 32;
> +        break;
> +    case MXL_RV64:
> +    case MXL_RV128:
> +        mxlen = 64;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    invalid_val = 1LL << (mxlen - 1);
> +
> +    if (value == invalid_val) {
> +        error_setg(errp, "Unable to set marchid with MSB (%u) bit set "
> +                         "and the remaining bits zero", mxlen);
> +        return;
> +    }
> +
> +    cpu->cfg.marchid = value;
> +}
> +
> +static void cpu_get_marchid(Object *obj, Visitor *v, const char *name,
> +                           void *opaque, Error **errp)
> +{
> +    bool value = RISCV_CPU(obj)->cfg.marchid;
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
>  static void riscv_cpu_class_init(ObjectClass *c, void *data)
>  {
>      RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> @@ -1918,6 +1961,9 @@ static void riscv_cpu_class_init(ObjectClass *c, void 
> *data)
>      object_class_property_add(c, "mimpid", "uint64", cpu_get_mimpid,
>                                cpu_set_mimpid, NULL, NULL);
>
> +    object_class_property_add(c, "marchid", "uint64", cpu_get_marchid,
> +                              cpu_set_marchid, NULL, NULL);
> +
>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>
> --
> 2.40.1
>
>