Re: [PATCH v2 10/33] target/riscv: Fix size of vector CSRs

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 10/1/25 12:32 AM, Anton Johansson wrote:
> According to version 20250508 of the unprivileged specification:
> - vtype: bits 0..7 used, bit XLEN-1 illegal, rest reserved
>    => fix to 64-bits.
> 
> - vxsat: bit 0 used, vxrm which would occupy bits 1..2 is stored
>    separately, and bits 3..31 are set to 0
>    => fix to 8-bits.
> 
> - vxrm: 2 lowest bits are used for rounding mode, rest set to 0
>    => fix to 8-bits.
> 
> - vstart: maximum value of VLMAX-1, where VLMAX is at most 2^16
>    => fix to 32-bits as vstart is mapped to a TCG global.
> 
> - vl: maximum value of VLEN which is at most 2^16
>    => fix to 32-bits as vl is mapped to a TCG global.
> 
> Fields are shuffled for reduced padding.
> 
> Note, the cpu/vector VMSTATE version is bumped, breaking migration from
> older versions.
> 
> Signed-off-by: Anton Johansson <anjo@rev.ng>
> ---
>   target/riscv/cpu.h                      | 12 ++++++------
>   target/riscv/machine.c                  | 14 +++++++-------
>   target/riscv/translate.c                | 12 ++++++++----
>   target/riscv/vector_helper.c            | 22 ++++++++++++++++++----
>   target/riscv/insn_trans/trans_rvv.c.inc | 22 +++++++++++-----------
>   5 files changed, 50 insertions(+), 32 deletions(-)
> 

...

>   typedef struct PMUCTRState {
>       /* Current value of a counter */
> @@ -217,11 +217,11 @@ struct CPUArchState {
>   
>       /* vector coprocessor state. */
>       uint64_t vreg[32 * RV_VLEN_MAX / 64] QEMU_ALIGNED(16);
> -    target_ulong vxrm;
> -    target_ulong vxsat;
> -    target_ulong vl;
> -    target_ulong vstart;
> -    target_ulong vtype;
> +    uint64_t vtype;
> +    uint32_t vl;
> +    uint32_t vstart;
> +    uint8_t vxrm;
> +    uint8_t vxsat;
>       bool vill;
>   
>       target_ulong pc;
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 6bc79cceaf..dcf18624bf 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -136,16 +136,16 @@ static bool vector_needed(void *opaque)
>   
>   static const VMStateDescription vmstate_vector = {
>       .name = "cpu/vector",
> -    .version_id = 2,
> -    .minimum_version_id = 2,
> +    .version_id = 3,
> +    .minimum_version_id = 3,
>       .needed = vector_needed,
>       .fields = (const VMStateField[]) {
>           VMSTATE_UINT64_ARRAY(env.vreg, RISCVCPU, 32 * RV_VLEN_MAX / 64),
> -        VMSTATE_UINTTL(env.vxrm, RISCVCPU),
> -        VMSTATE_UINTTL(env.vxsat, RISCVCPU),
> -        VMSTATE_UINTTL(env.vl, RISCVCPU),
> -        VMSTATE_UINTTL(env.vstart, RISCVCPU),
> -        VMSTATE_UINTTL(env.vtype, RISCVCPU),
> +        VMSTATE_UINT8(env.vxrm, RISCVCPU),
> +        VMSTATE_UINT8(env.vxsat, RISCVCPU),
> +        VMSTATE_UINT32(env.vl, RISCVCPU),
> +        VMSTATE_UINT32(env.vstart, RISCVCPU),
> +        VMSTATE_UINT64(env.vtype, RISCVCPU),


Just a nit, but might be more maintainable to keep in sync with fields
declaration order, given you changed that to optimize struct Cpu size.

>           VMSTATE_BOOL(env.vill, RISCVCPU),
>           VMSTATE_END_OF_LIST()
>       }

...

The rest is pretty straightforward,
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>