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