Re: [PATCH] RISC-V: Report the QEMU vendor/arch IDs on virtual CPUs

[Daniel Henrique Barboza]

On 1/31/24 15:24, Palmer Dabbelt wrote:
> Right now we just report 0 for marchid/mvendorid in QEMU.  That's legal,
> but it's tricky for users that want to check if they're running on QEMU
> to do so.  This sets marchid to 42, which I've proposed as the QEMU
> architecture ID (mvendorid remains 0, just explicitly set, as that's how
> the ISA handles open source implementations).
>
> Link: https://github.com/riscv/riscv-isa-manual/pull/1213
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> ---

This patch doesn't compile in my env:

../target/riscv/cpu.c: In function ‘riscv_any_cpu_init’:
../target/riscv/cpu.c:439:26: error: ‘QEMU_MVENDORID’ undeclared (first use in 
this function); did you mean ‘CSR_MVENDORID’?
  439 |     cpu->cfg.mvendorid = QEMU_MVENDORID;
      |                          ^~~~~~~~~~~~~~
      |                          CSR_MVENDORID
../target/riscv/cpu.c:439:26: note: each undeclared identifier is reported only 
once for each function it appears in
../target/riscv/cpu.c:440:24: error: ‘QEMU_MARCHID’ undeclared (first use in 
this function); did you mean ‘QEMU_ARCH’?
  440 |     cpu->cfg.marchid = QEMU_MARCHID;
      |                        ^~~~~~~~~~~~
      |                        QEMU_ARCH


In the patch where I changed 'marchid' (d6a427e2c0b2) I removed the following 
macro:

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 477f8f8f97..9080d021fa 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -39,11 +39,6 @@
 #include "tcg/tcg.h"
 
 /* RISC-V CPU definitions */
-
-#define RISCV_CPU_MARCHID   ((QEMU_VERSION_MAJOR << 16) | \
-                             (QEMU_VERSION_MINOR << 8)  | \
-                             (QEMU_VERSION_MICRO))
-

So I believe you can re-introduce it as '42' and use it. If you want to be 
really
explicit you can also re-introduce RISCV_CPU_MVENDORID and RISCV_CPU_MIMPID and
set them all to 0 and use them to assign all machine IDs.

The macro name doesn't matter but I'd not use 'VIRT' in the name. This marchid 
is
going to be used by any board that uses the CPU, not just the 'virt' board.

One more thing:


>   target/riscv/cpu.c          | 16 ++++++++++++++++
>   target/riscv/cpu_vendorid.h |  3 +++
>   2 files changed, 19 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 8cbfc7e781..1aef186f87 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -415,6 +415,9 @@ static void riscv_any_cpu_init(Object *obj)
>       cpu->cfg.ext_zicsr = true;
>       cpu->cfg.mmu = true;
>       cpu->cfg.pmp = true;
> +
> +    cpu->cfg.mvendorid = QEMU_MVENDORID;
> +    cpu->cfg.marchid = QEMU_MARCHID;


Instead of repeating this code in every cpu_init() I would just change 
post_init():

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 88e8cc8681..f6ef50bb20 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1293,6 +1293,12 @@ static bool riscv_cpu_is_dynamic(Object *cpu_obj)
 
 static void riscv_cpu_post_init(Object *obj)
 {
+    if (!riscv_cpu_is_vendor(obj)) {
+        RISCV_CPU(obj)->cfg.mvendorid = RISCV_CPU_MVENDORID;
+        RISCV_CPU(obj)->cfg.marchid = RISCV_CPU_MARCHID;
+        RISCV_CPU(obj)->cfg.mimpid = RISCV_CPU_MIMPID;
+    }
+
     accel_cpu_instance_init(CPU(obj));
 }

This will change the machine IDs for all CPUs that aren't vendor CPUs, which 
will
retain whatever value it was set in their cpu_init().


Thanks,


Daniel

>   }
>   
>   static void riscv_max_cpu_init(Object *obj)
> @@ -432,6 +435,8 @@ static void riscv_max_cpu_init(Object *obj)
>       set_satp_mode_max_supported(RISCV_CPU(obj), mlx == MXL_RV32 ?
>                                   VM_1_10_SV32 : VM_1_10_SV57);
>   #endif
> +    cpu->cfg.mvendorid = QEMU_MVENDORID;
> +    cpu->cfg.marchid = QEMU_MARCHID;
>   }
>   
>   #if defined(TARGET_RISCV64)
> @@ -445,6 +450,8 @@ static void rv64_base_cpu_init(Object *obj)
>   #ifndef CONFIG_USER_ONLY
>       set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
>   #endif
> +    cpu->cfg.mvendorid = QEMU_MVENDORID;
> +    cpu->cfg.marchid = QEMU_MARCHID;
>   }
>   
>   static void rv64_sifive_u_cpu_init(Object *obj)
> @@ -569,6 +576,8 @@ static void rv128_base_cpu_init(Object *obj)
>   #ifndef CONFIG_USER_ONLY
>       set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
>   #endif
> +    cpu->cfg.mvendorid = QEMU_MVENDORID;
> +    cpu->cfg.marchid = QEMU_MARCHID;
>   }
>   
>   static void rv64i_bare_cpu_init(Object *obj)
> @@ -591,6 +600,8 @@ static void rv64i_bare_cpu_init(Object *obj)
>   #ifndef CONFIG_USER_ONLY
>       set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV64);
>   #endif
> +    cpu->cfg.mvendorid = QEMU_MVENDORID;
> +    cpu->cfg.marchid = QEMU_MARCHID;
>   }
>   #else
>   static void rv32_base_cpu_init(Object *obj)
> @@ -603,6 +614,8 @@ static void rv32_base_cpu_init(Object *obj)
>   #ifndef CONFIG_USER_ONLY
>       set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
>   #endif
> +    cpu->cfg.mvendorid = QEMU_MVENDORID;
> +    cpu->cfg.marchid = QEMU_MARCHID;
>   }
>   
>   static void rv32_sifive_u_cpu_init(Object *obj)
> @@ -672,6 +685,9 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
>       cpu->cfg.ext_zifencei = true;
>       cpu->cfg.ext_zicsr = true;
>       cpu->cfg.pmp = true;
> +
> +    cpu->cfg.mvendorid = QEMU_MVENDORID;
> +    cpu->cfg.marchid = QEMU_MARCHID;
>   }
>   #endif
>   
> diff --git a/target/riscv/cpu_vendorid.h b/target/riscv/cpu_vendorid.h
> index 96b6b9c2cb..486832cd53 100644
> --- a/target/riscv/cpu_vendorid.h
> +++ b/target/riscv/cpu_vendorid.h
> @@ -7,4 +7,7 @@
>   #define VEYRON_V1_MIMPID        0x111
>   #define VEYRON_V1_MVENDORID     0x61f
>   
> +#define QEMU_VIRT_MVENDORID     0
> +#define QEMU_VIRT_MARCHID       42
> +
>   #endif /*  TARGET_RISCV_CPU_VENDORID_H */