Hi Drew, Thanks a lot for the detailed review and suggestions. I agree with your points.
I will post a revised version in about a week. This is mainly to allow some time in case other reviewers have additional feedback on the current approach, so that everything can be addressed together in the next revision. Thanks again for the careful review. Best regards, Jiakai On Thu, Feb 5, 2026 at 11:58 PM Andrew Jones <[email protected]> wrote: > > On Thu, Feb 05, 2026 at 01:05:02AM +0000, Jiakai Xu wrote: > > Add RISC-V KVM selftests to verify the SBI Steal-Time Accounting (STA) > > shared memory alignment requirements. > > > > The SBI specification requires the STA shared memory GPA to be 64-byte > > aligned, or set to all-ones to explicitly disable steal-time accounting. > > This test verifies that KVM enforces the expected behavior when > > configuring the SBI STA shared memory via KVM_SET_ONE_REG. > > > > Specifically, the test checks that: > > - misaligned GPAs are rejected with -EINVAL > > - 64-byte aligned GPAs are accepted > > - INVALID_GPA correctly disables steal-time accounting > > > > Signed-off-by: Jiakai Xu <[email protected]> > > Signed-off-by: Jiakai Xu <[email protected]> > > --- > > .../selftests/kvm/include/riscv/processor.h | 4 +++ > > tools/testing/selftests/kvm/steal_time.c | 33 +++++++++++++++++++ > > 2 files changed, 37 insertions(+) > > > > diff --git a/tools/testing/selftests/kvm/include/riscv/processor.h > > b/tools/testing/selftests/kvm/include/riscv/processor.h > > index e58282488beb3..c3551d129d2f6 100644 > > --- a/tools/testing/selftests/kvm/include/riscv/processor.h > > +++ b/tools/testing/selftests/kvm/include/riscv/processor.h > > @@ -62,6 +62,10 @@ static inline uint64_t __kvm_reg_id(uint64_t type, > > uint64_t subtype, > > > > KVM_REG_RISCV_SBI_SINGLE, \ > > idx, KVM_REG_SIZE_ULONG) > > > > +#define RISCV_SBI_STA_REG(idx) __kvm_reg_id(KVM_REG_RISCV_SBI_STATE, > > \ > > + KVM_REG_RISCV_SBI_STA, > > \ > > + idx, KVM_REG_SIZE_ULONG) > > We don't need this because... > > > + > > bool __vcpu_has_ext(struct kvm_vcpu *vcpu, uint64_t ext); > > > > static inline bool __vcpu_has_isa_ext(struct kvm_vcpu *vcpu, uint64_t > > isa_ext) > > diff --git a/tools/testing/selftests/kvm/steal_time.c > > b/tools/testing/selftests/kvm/steal_time.c > > index 8edc1fca345ba..30b98d1b601c3 100644 > > --- a/tools/testing/selftests/kvm/steal_time.c > > +++ b/tools/testing/selftests/kvm/steal_time.c > > @@ -209,6 +209,7 @@ static void steal_time_dump(struct kvm_vm *vm, uint32_t > > vcpu_idx) > > > > /* SBI STA shmem must have 64-byte alignment */ > > #define STEAL_TIME_SIZE ((sizeof(struct sta_struct) + 63) & > > ~63) > > +#define INVALID_GPA (~(u64)0) > > > > static vm_paddr_t st_gpa[NR_VCPUS]; > > > > @@ -301,6 +302,34 @@ static void steal_time_dump(struct kvm_vm *vm, > > uint32_t vcpu_idx) > > pr_info("\n"); > > } > > > > +static void test_riscv_sta_shmem_alignment(struct kvm_vcpu *vcpu) > > +{ > > + struct kvm_one_reg reg; > > + uint64_t shmem; > > + int ret; > > + > > + reg.id = RISCV_SBI_STA_REG(0); > > ...here we should use KVM_REG_RISCV_SBI_STA_REG(shmem_lo) > > > + reg.addr = (uint64_t)&shmem; > > + > > + /* Case 1: misaligned GPA */ > > + shmem = ST_GPA_BASE + 1; > > + ret = __vcpu_ioctl(vcpu, KVM_SET_ONE_REG, ®); > > + TEST_ASSERT(ret == -1 && errno == EINVAL, > > + "misaligned STA shmem should return -EINVAL"); > > remove the word 'should' > > "misaligned STA shmem returns -EINVAL" > > > + > > + /* Case 2: 64-byte aligned GPA */ > > + shmem = ST_GPA_BASE; > > + ret = __vcpu_ioctl(vcpu, KVM_SET_ONE_REG, ®); > > + TEST_ASSERT(ret == 0, > > + "aligned STA shmem should succeed"); > > same comment about 'should' > > > + > > + /* Case 3: INVALID_GPA disables STA */ > > + shmem = INVALID_GPA; > > + ret = __vcpu_ioctl(vcpu, KVM_SET_ONE_REG, ®); > > + TEST_ASSERT(ret == 0, > > + "INVALID_GPA should disable STA successfully"); > > We're not testing that STA was successfully disabled, only that all-ones > input doesn't generate an error. So the message should be along the lines > of "all-ones for STA shmem succeeds" > > > +} > > + > > #endif > > > > static void *do_steal_time(void *arg) > > @@ -369,6 +398,10 @@ int main(int ac, char **av) > > TEST_REQUIRE(is_steal_time_supported(vcpus[0])); > > ksft_set_plan(NR_VCPUS); > > > > +#ifdef __riscv > > + test_riscv_sta_shmem_alignment(vcpus[0]); > > +#endif > > We like to try to avoid #ifdefs in common functions by providing stubs for > architectures that don't need them [yet]. So we should rename this to > something more generic, like > > check_steal_time_uapi() > > and then call it for all architectures. Actually the other architectures > can already make use of it since both x86 and arm64 do uapi tests in > their steal_time_init() functions and that's not really the right > place to do that. I suggest creating another patch that first moves those > tests into new functions (check_steal_time_uapi()) which only needs to > be called once for vcpu[0] outside the vcpu loop, as you do here. In > that patch check_steal_time_uapi() will be a stub for riscv. Then in > a second patch fill in that stub with the tests above. > > Thanks, > drew > > > + > > /* Run test on each VCPU */ > > for (i = 0; i < NR_VCPUS; ++i) { > > steal_time_init(vcpus[i], i); > > -- > > 2.34.1 > >
