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;
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),
VMSTATE_UINT32(env.scountinhibit, RISCVCPU),
VMSTATE_UINT32(env.mcountinhibit, RISCVCPU),
VMSTATE_STRUCT_ARRAY(env.pmu_ctrs, RISCVCPU, RV_MAX_MHPMCOUNTERS, 0,
--
2.49.0On 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
On 4/25/25 9:11 AM, Andrew Jones wrote:
> 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.
Fair enough.
>
>>
>> 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?
The VMSTATE_UINT32() macro checks for the size of the variable and will fail in compile
time if it's not an uint32_t:
In file included from /home/danielhb/work/qemu/include/qemu/osdep.h:53,
from ../target/riscv/machine.c:19:
/home/danielhb/work/qemu/include/qemu/compiler.h:65:35: error: invalid operands to binary - (have ‘uint32_t *’ {aka ‘unsigned int *’} and ‘target_ulong *’ {aka ‘long unsigned int *’})
65 | #define type_check(t1,t2) ((t1*)0 - (t2*)0)
| ^
/home/danielhb/work/qemu/include/migration/vmstate.h:270:6: note: in expansion of macro ‘type_check’
270 | type_check(_type, typeof_field(_state, _field)))
| ^~~~~~~~~~
/home/danielhb/work/qemu/include/migration/vmstate.h:321:21: note: in expansion of macro ‘vmstate_offset_value’
321 | .offset = vmstate_offset_value(_state, _field, _type), \
| ^~~~~~~~~~~~~~~~~~~~
/home/danielhb/work/qemu/include/migration/vmstate.h:854:5: note: in expansion of macro ‘VMSTATE_SINGLE_TEST’
854 | VMSTATE_SINGLE_TEST(_field, _state, NULL, _version, _info, _type)
| ^~~~~~~~~~~~~~~~~~~
/home/danielhb/work/qemu/include/migration/vmstate.h:902:5: note: in expansion of macro ‘VMSTATE_SINGLE’
902 | VMSTATE_SINGLE(_f, _s, _v, vmstate_info_uint32, uint32_t)
| ^~~~~~~~~~~~~~
/home/danielhb/work/qemu/include/migration/vmstate.h:939:5: note: in expansion of macro ‘VMSTATE_UINT32_V’
939 | VMSTATE_UINT32_V(_f, _s, 0)
| ^~~~~~~~~~~~~~~~
../target/riscv/machine.c:448:9: note: in expansion of macro ‘VMSTATE_UINT32’
448 | VMSTATE_UINT32(env.scounteren, RISCVCPU),
| ^~~~~~~~~~~~~~
Thanks,
Daniel
>
>> 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
© 2016 - 2025 Red Hat, Inc.