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

[Andrew Jones]

On Tue, May 30, 2023 at 04:46:12PM -0300, Daniel Henrique Barboza 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>
> ---
>  target/riscv/cpu.c | 53 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index ed3b33343c..d6e23bfd83 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 {
> @@ -1722,8 +1717,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
> @@ -1854,6 +1847,48 @@ static void cpu_set_mimpid(Object *obj, Visitor *v, 
> const char *name,
>      cpu->cfg.mimpid = value;
>  }
>  
> +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 riscv_cpu_class_init(ObjectClass *c, void *data)
>  {
>      RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> @@ -1893,6 +1928,10 @@ static void riscv_cpu_class_init(ObjectClass *c, void 
> *data)
>                                cpu_set_mimpid,
>                                NULL, NULL);
>  
> +    object_class_property_add(c, "marchid", "uint64", NULL,
> +                              cpu_set_marchid,
> +                              NULL, NULL);
> +

get?

>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>  
> -- 
> 2.40.1
> 
>

Otherwise,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>