Re: [PATCH v2 8/9] target/riscv: widen (m|s)counteren to target_ulong

[Andrew Jones <ajones@ventanamicro.com>]

On Fri, Apr 25, 2025 at 08:37:04AM -0300, Daniel Henrique Barboza wrote:
> We want to support scounteren as a KVM CSR. The KVM UAPI defines every
> CSR size as target_ulong, and our env->scounteren is fixed at 32 bits.
> 
> The other existing cases where the property size does not match the KVM
> reg size happens with uint64_t properties, like 'mstatus'. When running
> a 32 bit CPU we'll write a 32 bit 'sstatus' KVM reg into the 64 bit
> 'mstatus' field. As long as we're consistent, i.e. we're always
> reading/writing the same words, this is ok.
> 
> For scounteren, a KVM guest running in a 64 bit CPU will end up writing
> a 64 bit reg in a 32 bit field. This will have all sort of funny side
> effects in the KVM guest that we would rather avoid.
> 
> Increase scounteren to target_ulong to allow KVM to read/write the
> scounteren CSR without any surprises. 'mcounteren' is being changed to
> target_ulong for consistency.
> 
> Aside from bumping the version of the RISCVCPU vmstate no other
> behavioral changes are expected.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.h     | 4 ++--
>  target/riscv/machine.c | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index f5a60d0c52..0623c3187b 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -400,8 +400,8 @@ struct CPUArchState {
>       */
>      bool two_stage_indirect_lookup;
>  
> -    uint32_t scounteren;
> -    uint32_t mcounteren;
> +    target_ulong scounteren;
> +    target_ulong mcounteren;

Let's leave mcounteren a u32 and write a comment above scounteren
explaining that it's supposed to be a u32 (as the spec says) but
we're using a ulong instead to support KVM's get/put due to how
it's defined in struct kvm_riscv_csr.

>  
>      uint32_t scountinhibit;
>      uint32_t mcountinhibit;
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index df2d5bad8d..4b11b203fb 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -401,8 +401,8 @@ static const VMStateDescription vmstate_ssp = {
>  
>  const VMStateDescription vmstate_riscv_cpu = {
>      .name = "cpu",
> -    .version_id = 10,
> -    .minimum_version_id = 10,
> +    .version_id = 11,
> +    .minimum_version_id = 11,
>      .post_load = riscv_cpu_post_load,
>      .fields = (const VMStateField[]) {
>          VMSTATE_UINTTL_ARRAY(env.gpr, RISCVCPU, 32),
> @@ -445,8 +445,8 @@ const VMStateDescription vmstate_riscv_cpu = {
>          VMSTATE_UINTTL(env.mtval, RISCVCPU),
>          VMSTATE_UINTTL(env.miselect, RISCVCPU),
>          VMSTATE_UINTTL(env.siselect, RISCVCPU),
> -        VMSTATE_UINT32(env.scounteren, RISCVCPU),
> -        VMSTATE_UINT32(env.mcounteren, RISCVCPU),
> +        VMSTATE_UINTTL(env.scounteren, RISCVCPU),
> +        VMSTATE_UINTTL(env.mcounteren, RISCVCPU),

Since we only expect the lower 32 bits to ever be written, then do we need
to make this change?

>          VMSTATE_UINT32(env.scountinhibit, RISCVCPU),
>          VMSTATE_UINT32(env.mcountinhibit, RISCVCPU),
>          VMSTATE_STRUCT_ARRAY(env.pmu_ctrs, RISCVCPU, RV_MAX_MHPMCOUNTERS, 0,
> -- 
> 2.49.0
> 

Thanks,
drew