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 <dbarb...@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
