| Message ID | 20240403080452.1007601-22-atishp@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISC-V SBI v2.0 PMU improvements and Perf sampling in KVM guest | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
On Wed, Apr 03, 2024 at 01:04:50AM -0700, Atish Patra wrote: > Verify PMU snapshot functionality by setting up the shared memory > correctly and reading the counter values from the shared memory > instead of the CSR. > > Reviewed-by: Anup Patel <anup@brainfault.org> > Signed-off-by: Atish Patra <atishp@rivosinc.com> > --- > .../testing/selftests/kvm/include/riscv/sbi.h | 25 ++++ > .../selftests/kvm/lib/riscv/processor.c | 12 ++ > .../selftests/kvm/riscv/sbi_pmu_test.c | 127 ++++++++++++++++++ > 3 files changed, 164 insertions(+) > > diff --git a/tools/testing/selftests/kvm/include/riscv/sbi.h b/tools/testing/selftests/kvm/include/riscv/sbi.h > index 6675ca673c77..8c98bd99d450 100644 > --- a/tools/testing/selftests/kvm/include/riscv/sbi.h > +++ b/tools/testing/selftests/kvm/include/riscv/sbi.h > @@ -8,6 +8,12 @@ > #ifndef SELFTEST_KVM_SBI_H > #define SELFTEST_KVM_SBI_H > > +/* SBI spec version fields */ > +#define SBI_SPEC_VERSION_DEFAULT 0x1 > +#define SBI_SPEC_VERSION_MAJOR_SHIFT 24 > +#define SBI_SPEC_VERSION_MAJOR_MASK 0x7f > +#define SBI_SPEC_VERSION_MINOR_MASK 0xffffff > + > /* SBI return error codes */ > #define SBI_SUCCESS 0 > #define SBI_ERR_FAILURE -1 > @@ -33,6 +39,9 @@ enum sbi_ext_id { > }; > > enum sbi_ext_base_fid { > + SBI_EXT_BASE_GET_SPEC_VERSION = 0, > + SBI_EXT_BASE_GET_IMP_ID, > + SBI_EXT_BASE_GET_IMP_VERSION, > SBI_EXT_BASE_PROBE_EXT = 3, > }; > enum sbi_ext_pmu_fid { > @@ -60,6 +69,12 @@ union sbi_pmu_ctr_info { > }; > }; > > +struct riscv_pmu_snapshot_data { > + u64 ctr_overflow_mask; > + u64 ctr_values[64]; > + u64 reserved[447]; > +}; > + > struct sbiret { > long error; > long value; > @@ -113,4 +128,14 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0, > > bool guest_sbi_probe_extension(int extid, long *out_val); > > +/* Make SBI version */ > +static inline unsigned long sbi_mk_version(unsigned long major, > + unsigned long minor) > +{ > + return ((major & SBI_SPEC_VERSION_MAJOR_MASK) << > + SBI_SPEC_VERSION_MAJOR_SHIFT) | minor; Should also also mask 'minor'. I see this matches what we have in the kernel so we should fix it there too. > +} > + > +unsigned long get_host_sbi_spec_version(void); > + > #endif /* SELFTEST_KVM_SBI_H */ > diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c > index e8211f5d6863..ccb35573749c 100644 > --- a/tools/testing/selftests/kvm/lib/riscv/processor.c > +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c > @@ -502,3 +502,15 @@ bool guest_sbi_probe_extension(int extid, long *out_val) > > return true; > } > + > +unsigned long get_host_sbi_spec_version(void) > +{ > + struct sbiret ret; > + > + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_SPEC_VERSION, 0, > + 0, 0, 0, 0, 0); > + > + GUEST_ASSERT(!ret.error); > + > + return ret.value; > +} > diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c > index 8e7c7a3172d8..7d195be5c3d9 100644 > --- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c > +++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c > @@ -19,6 +19,11 @@ > #define RISCV_MAX_PMU_COUNTERS 64 > union sbi_pmu_ctr_info ctrinfo_arr[RISCV_MAX_PMU_COUNTERS]; > > +/* Snapshot shared memory data */ > +#define PMU_SNAPSHOT_GPA_BASE BIT(30) > +static void *snapshot_gva; > +static vm_paddr_t snapshot_gpa; > + > /* Cache the available counters in a bitmask */ > static unsigned long counter_mask_available; > > @@ -178,6 +183,32 @@ static unsigned long read_counter(int idx, union sbi_pmu_ctr_info ctrinfo) > return counter_val; > } > > +static inline void verify_sbi_requirement_assert(void) > +{ > + long out_val = 0; > + bool probe; > + > + probe = guest_sbi_probe_extension(SBI_EXT_PMU, &out_val); > + GUEST_ASSERT(probe && out_val == 1); > + > + if (get_host_sbi_spec_version() < sbi_mk_version(2, 0)) > + __GUEST_ASSERT(0, "SBI implementation version doesn't support PMU Snapshot"); > +} It's a pity we can't check the SBI spec version that KVM is advertising from KVM userspace. Normally we'd want to check something like this at the start of the test with TEST_REQUIRE() before running a VCPU in order to generate a skip exit. (We probably should allow reading and even writing the SBI spec version from the VMM in order to better support migration.) > + > +static void snapshot_set_shmem(vm_paddr_t gpa, unsigned long flags) > +{ > + unsigned long lo = (unsigned long)gpa; > +#if __riscv_xlen == 32 > + unsigned long hi = (unsigned long)(gpa >> 32); > +#else > + unsigned long hi = gpa == -1 ? -1 : 0; > +#endif > + struct sbiret ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_SNAPSHOT_SET_SHMEM, > + lo, hi, flags, 0, 0, 0); > + > + GUEST_ASSERT(ret.value == 0 && ret.error == 0); > +} > + > static void test_pmu_event(unsigned long event) > { > unsigned long counter; > @@ -210,6 +241,41 @@ static void test_pmu_event(unsigned long event) > stop_reset_counter(counter, 0); > } > > +static void test_pmu_event_snapshot(unsigned long event) > +{ > + unsigned long counter; > + unsigned long counter_value_pre, counter_value_post; > + unsigned long counter_init_value = 100; > + struct riscv_pmu_snapshot_data *snapshot_data = snapshot_gva; > + > + counter = get_counter_index(0, counter_mask_available, 0, event); > + counter_value_pre = read_counter(counter, ctrinfo_arr[counter]); > + > + /* Do not set the initial value */ > + start_counter(counter, 0, 0); > + dummy_func_loop(10000); > + stop_counter(counter, SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT); > + > + /* The counter value is updated w.r.t relative index of cbase */ > + counter_value_post = READ_ONCE(snapshot_data->ctr_values[0]); > + __GUEST_ASSERT(counter_value_post > counter_value_pre, > + "counter_value_post %lx counter_value_pre %lx\n", > + counter_value_post, counter_value_pre); > + > + /* Now set the initial value and compare */ > + WRITE_ONCE(snapshot_data->ctr_values[0], counter_init_value); > + start_counter(counter, SBI_PMU_START_FLAG_INIT_SNAPSHOT, 0); > + dummy_func_loop(10000); > + stop_counter(counter, SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT); > + > + counter_value_post = READ_ONCE(snapshot_data->ctr_values[0]); > + __GUEST_ASSERT(counter_value_post > counter_init_value, > + "counter_value_post %lx counter_init_value %lx for counter\n", > + counter_value_post, counter_init_value); > + > + stop_reset_counter(counter, 0); > +} > + > static void test_invalid_event(void) > { > struct sbiret ret; > @@ -272,6 +338,34 @@ static void test_pmu_basic_sanity(void) > GUEST_DONE(); > } > > +static void test_pmu_events_snaphost(void) > +{ > + int num_counters = 0; > + struct riscv_pmu_snapshot_data *snapshot_data = snapshot_gva; > + int i; > + > + /* Verify presence of SBI PMU and minimum requrired SBI version */ > + verify_sbi_requirement_assert(); > + > + snapshot_set_shmem(snapshot_gpa, 0); > + > + /* Get the counter details */ > + num_counters = get_num_counters(); > + update_counter_info(num_counters); > + > + /* Validate shared memory access */ > + GUEST_ASSERT_EQ(READ_ONCE(snapshot_data->ctr_overflow_mask), 0); > + for (i = 0; i < num_counters; i++) { > + if (counter_mask_available & (BIT(i))) > + GUEST_ASSERT_EQ(READ_ONCE(snapshot_data->ctr_values[i]), 0); > + } > + /* Only these two events are guranteed to be present */ > + test_pmu_event_snapshot(SBI_PMU_HW_CPU_CYCLES); > + test_pmu_event_snapshot(SBI_PMU_HW_INSTRUCTIONS); > + > + GUEST_DONE(); > +} > + > static void run_vcpu(struct kvm_vcpu *vcpu) > { > struct ucall uc; > @@ -328,13 +422,46 @@ static void test_vm_events_test(void *guest_code) > test_vm_destroy(vm); > } > > +static void test_vm_setup_snapshot_mem(struct kvm_vm *vm, struct kvm_vcpu *vcpu) > +{ > + /* PMU Snapshot requires single page only */ > + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, PMU_SNAPSHOT_GPA_BASE, 1, 1, 0); > + /* PMU_SNAPSHOT_GPA_BASE is identity mapped */ > + virt_map(vm, PMU_SNAPSHOT_GPA_BASE, PMU_SNAPSHOT_GPA_BASE, 1); > + > + snapshot_gva = (void *)(PMU_SNAPSHOT_GPA_BASE); > + snapshot_gpa = addr_gva2gpa(vcpu->vm, (vm_vaddr_t)snapshot_gva); > + sync_global_to_guest(vcpu->vm, snapshot_gva); > + sync_global_to_guest(vcpu->vm, snapshot_gpa); > +} > + > +static void test_vm_events_snapshot_test(void *guest_code) > +{ > + struct kvm_vm *vm = NULL; > + struct kvm_vcpu *vcpu; > + > + vm = vm_create_with_one_vcpu(&vcpu, guest_code); > + __TEST_REQUIRE(__vcpu_has_sbi_ext(vcpu, KVM_RISCV_SBI_EXT_PMU), > + "SBI PMU not available, skipping test"); > + > + test_vm_setup_snapshot_mem(vm, vcpu); > + > + run_vcpu(vcpu); > + > + test_vm_destroy(vm); > +} > + > int main(void) > { > + pr_info("SBI PMU basic test : starting\n"); > test_vm_basic_test(test_pmu_basic_sanity); > pr_info("SBI PMU basic test : PASS\n"); > > test_vm_events_test(test_pmu_events); > pr_info("SBI PMU event verification test : PASS\n"); > > + test_vm_events_snapshot_test(test_pmu_events_snaphost); > + pr_info("SBI PMU event verification with snapshot test : PASS\n"); > + > return 0; > } > -- > 2.34.1 > Since my comments are a bit out-of-scope for this patch, Reviewed-by: Andrew Jones <ajones@ventanamicro.com> Thanks, drew
On 4/5/24 06:11, Andrew Jones wrote: > On Wed, Apr 03, 2024 at 01:04:50AM -0700, Atish Patra wrote: >> Verify PMU snapshot functionality by setting up the shared memory >> correctly and reading the counter values from the shared memory >> instead of the CSR. >> >> Reviewed-by: Anup Patel <anup@brainfault.org> >> Signed-off-by: Atish Patra <atishp@rivosinc.com> >> --- >> .../testing/selftests/kvm/include/riscv/sbi.h | 25 ++++ >> .../selftests/kvm/lib/riscv/processor.c | 12 ++ >> .../selftests/kvm/riscv/sbi_pmu_test.c | 127 ++++++++++++++++++ >> 3 files changed, 164 insertions(+) >> >> diff --git a/tools/testing/selftests/kvm/include/riscv/sbi.h b/tools/testing/selftests/kvm/include/riscv/sbi.h >> index 6675ca673c77..8c98bd99d450 100644 >> --- a/tools/testing/selftests/kvm/include/riscv/sbi.h >> +++ b/tools/testing/selftests/kvm/include/riscv/sbi.h >> @@ -8,6 +8,12 @@ >> #ifndef SELFTEST_KVM_SBI_H >> #define SELFTEST_KVM_SBI_H >> >> +/* SBI spec version fields */ >> +#define SBI_SPEC_VERSION_DEFAULT 0x1 >> +#define SBI_SPEC_VERSION_MAJOR_SHIFT 24 >> +#define SBI_SPEC_VERSION_MAJOR_MASK 0x7f >> +#define SBI_SPEC_VERSION_MINOR_MASK 0xffffff >> + >> /* SBI return error codes */ >> #define SBI_SUCCESS 0 >> #define SBI_ERR_FAILURE -1 >> @@ -33,6 +39,9 @@ enum sbi_ext_id { >> }; >> >> enum sbi_ext_base_fid { >> + SBI_EXT_BASE_GET_SPEC_VERSION = 0, >> + SBI_EXT_BASE_GET_IMP_ID, >> + SBI_EXT_BASE_GET_IMP_VERSION, >> SBI_EXT_BASE_PROBE_EXT = 3, >> }; >> enum sbi_ext_pmu_fid { >> @@ -60,6 +69,12 @@ union sbi_pmu_ctr_info { >> }; >> }; >> >> +struct riscv_pmu_snapshot_data { >> + u64 ctr_overflow_mask; >> + u64 ctr_values[64]; >> + u64 reserved[447]; >> +}; >> + >> struct sbiret { >> long error; >> long value; >> @@ -113,4 +128,14 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0, >> >> bool guest_sbi_probe_extension(int extid, long *out_val); >> >> +/* Make SBI version */ >> +static inline unsigned long sbi_mk_version(unsigned long major, >> + unsigned long minor) >> +{ >> + return ((major & SBI_SPEC_VERSION_MAJOR_MASK) << >> + SBI_SPEC_VERSION_MAJOR_SHIFT) | minor; > > Should also also mask 'minor'. I see this matches what we have in the > kernel so we should fix it there too. > Nice catch. I fixed it in both places. >> +} >> + >> +unsigned long get_host_sbi_spec_version(void); >> + >> #endif /* SELFTEST_KVM_SBI_H */ >> diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c >> index e8211f5d6863..ccb35573749c 100644 >> --- a/tools/testing/selftests/kvm/lib/riscv/processor.c >> +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c >> @@ -502,3 +502,15 @@ bool guest_sbi_probe_extension(int extid, long *out_val) >> >> return true; >> } >> + >> +unsigned long get_host_sbi_spec_version(void) >> +{ >> + struct sbiret ret; >> + >> + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_SPEC_VERSION, 0, >> + 0, 0, 0, 0, 0); >> + >> + GUEST_ASSERT(!ret.error); >> + >> + return ret.value; >> +} >> diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c >> index 8e7c7a3172d8..7d195be5c3d9 100644 >> --- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c >> +++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c >> @@ -19,6 +19,11 @@ >> #define RISCV_MAX_PMU_COUNTERS 64 >> union sbi_pmu_ctr_info ctrinfo_arr[RISCV_MAX_PMU_COUNTERS]; >> >> +/* Snapshot shared memory data */ >> +#define PMU_SNAPSHOT_GPA_BASE BIT(30) >> +static void *snapshot_gva; >> +static vm_paddr_t snapshot_gpa; >> + >> /* Cache the available counters in a bitmask */ >> static unsigned long counter_mask_available; >> >> @@ -178,6 +183,32 @@ static unsigned long read_counter(int idx, union sbi_pmu_ctr_info ctrinfo) >> return counter_val; >> } >> >> +static inline void verify_sbi_requirement_assert(void) >> +{ >> + long out_val = 0; >> + bool probe; >> + >> + probe = guest_sbi_probe_extension(SBI_EXT_PMU, &out_val); >> + GUEST_ASSERT(probe && out_val == 1); >> + >> + if (get_host_sbi_spec_version() < sbi_mk_version(2, 0)) >> + __GUEST_ASSERT(0, "SBI implementation version doesn't support PMU Snapshot"); >> +} > > It's a pity we can't check the SBI spec version that KVM is advertising > from KVM userspace. Normally we'd want to check something like this at > the start of the test with TEST_REQUIRE() before running a VCPU in order > to generate a skip exit. > Agreed. I will send a separate series for that as it is an ABI change. > (We probably should allow reading and even writing the SBI spec version > from the VMM in order to better support migration.) > How that would work for SBI spec version write use case ? For migraiton, you can't go back to older SBI versions in the host. Isn't it ? Considering this case your VM is running with PMU snapshot as the host has SBI v2.0. It can't be migrated to v1.0 and expecting it work. Correct ? >> + >> +static void snapshot_set_shmem(vm_paddr_t gpa, unsigned long flags) >> +{ >> + unsigned long lo = (unsigned long)gpa; >> +#if __riscv_xlen == 32 >> + unsigned long hi = (unsigned long)(gpa >> 32); >> +#else >> + unsigned long hi = gpa == -1 ? -1 : 0; >> +#endif >> + struct sbiret ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_SNAPSHOT_SET_SHMEM, >> + lo, hi, flags, 0, 0, 0); >> + >> + GUEST_ASSERT(ret.value == 0 && ret.error == 0); >> +} >> + >> static void test_pmu_event(unsigned long event) >> { >> unsigned long counter; >> @@ -210,6 +241,41 @@ static void test_pmu_event(unsigned long event) >> stop_reset_counter(counter, 0); >> } >> >> +static void test_pmu_event_snapshot(unsigned long event) >> +{ >> + unsigned long counter; >> + unsigned long counter_value_pre, counter_value_post; >> + unsigned long counter_init_value = 100; >> + struct riscv_pmu_snapshot_data *snapshot_data = snapshot_gva; >> + >> + counter = get_counter_index(0, counter_mask_available, 0, event); >> + counter_value_pre = read_counter(counter, ctrinfo_arr[counter]); >> + >> + /* Do not set the initial value */ >> + start_counter(counter, 0, 0); >> + dummy_func_loop(10000); >> + stop_counter(counter, SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT); >> + >> + /* The counter value is updated w.r.t relative index of cbase */ >> + counter_value_post = READ_ONCE(snapshot_data->ctr_values[0]); >> + __GUEST_ASSERT(counter_value_post > counter_value_pre, >> + "counter_value_post %lx counter_value_pre %lx\n", >> + counter_value_post, counter_value_pre); >> + >> + /* Now set the initial value and compare */ >> + WRITE_ONCE(snapshot_data->ctr_values[0], counter_init_value); >> + start_counter(counter, SBI_PMU_START_FLAG_INIT_SNAPSHOT, 0); >> + dummy_func_loop(10000); >> + stop_counter(counter, SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT); >> + >> + counter_value_post = READ_ONCE(snapshot_data->ctr_values[0]); >> + __GUEST_ASSERT(counter_value_post > counter_init_value, >> + "counter_value_post %lx counter_init_value %lx for counter\n", >> + counter_value_post, counter_init_value); >> + >> + stop_reset_counter(counter, 0); >> +} >> + >> static void test_invalid_event(void) >> { >> struct sbiret ret; >> @@ -272,6 +338,34 @@ static void test_pmu_basic_sanity(void) >> GUEST_DONE(); >> } >> >> +static void test_pmu_events_snaphost(void) >> +{ >> + int num_counters = 0; >> + struct riscv_pmu_snapshot_data *snapshot_data = snapshot_gva; >> + int i; >> + >> + /* Verify presence of SBI PMU and minimum requrired SBI version */ >> + verify_sbi_requirement_assert(); >> + >> + snapshot_set_shmem(snapshot_gpa, 0); >> + >> + /* Get the counter details */ >> + num_counters = get_num_counters(); >> + update_counter_info(num_counters); >> + >> + /* Validate shared memory access */ >> + GUEST_ASSERT_EQ(READ_ONCE(snapshot_data->ctr_overflow_mask), 0); >> + for (i = 0; i < num_counters; i++) { >> + if (counter_mask_available & (BIT(i))) >> + GUEST_ASSERT_EQ(READ_ONCE(snapshot_data->ctr_values[i]), 0); >> + } >> + /* Only these two events are guranteed to be present */ >> + test_pmu_event_snapshot(SBI_PMU_HW_CPU_CYCLES); >> + test_pmu_event_snapshot(SBI_PMU_HW_INSTRUCTIONS); >> + >> + GUEST_DONE(); >> +} >> + >> static void run_vcpu(struct kvm_vcpu *vcpu) >> { >> struct ucall uc; >> @@ -328,13 +422,46 @@ static void test_vm_events_test(void *guest_code) >> test_vm_destroy(vm); >> } >> >> +static void test_vm_setup_snapshot_mem(struct kvm_vm *vm, struct kvm_vcpu *vcpu) >> +{ >> + /* PMU Snapshot requires single page only */ >> + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, PMU_SNAPSHOT_GPA_BASE, 1, 1, 0); >> + /* PMU_SNAPSHOT_GPA_BASE is identity mapped */ >> + virt_map(vm, PMU_SNAPSHOT_GPA_BASE, PMU_SNAPSHOT_GPA_BASE, 1); >> + >> + snapshot_gva = (void *)(PMU_SNAPSHOT_GPA_BASE); >> + snapshot_gpa = addr_gva2gpa(vcpu->vm, (vm_vaddr_t)snapshot_gva); >> + sync_global_to_guest(vcpu->vm, snapshot_gva); >> + sync_global_to_guest(vcpu->vm, snapshot_gpa); >> +} >> + >> +static void test_vm_events_snapshot_test(void *guest_code) >> +{ >> + struct kvm_vm *vm = NULL; >> + struct kvm_vcpu *vcpu; >> + >> + vm = vm_create_with_one_vcpu(&vcpu, guest_code); >> + __TEST_REQUIRE(__vcpu_has_sbi_ext(vcpu, KVM_RISCV_SBI_EXT_PMU), >> + "SBI PMU not available, skipping test"); >> + >> + test_vm_setup_snapshot_mem(vm, vcpu); >> + >> + run_vcpu(vcpu); >> + >> + test_vm_destroy(vm); >> +} >> + >> int main(void) >> { >> + pr_info("SBI PMU basic test : starting\n"); >> test_vm_basic_test(test_pmu_basic_sanity); >> pr_info("SBI PMU basic test : PASS\n"); >> >> test_vm_events_test(test_pmu_events); >> pr_info("SBI PMU event verification test : PASS\n"); >> >> + test_vm_events_snapshot_test(test_pmu_events_snaphost); >> + pr_info("SBI PMU event verification with snapshot test : PASS\n"); >> + >> return 0; >> } >> -- >> 2.34.1 >> > > Since my comments are a bit out-of-scope for this patch, > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > Thanks, > drew
On Tue, Apr 09, 2024 at 03:52:40PM -0700, Atish Patra wrote: > On 4/5/24 06:11, Andrew Jones wrote: > > On Wed, Apr 03, 2024 at 01:04:50AM -0700, Atish Patra wrote: ... > > > + probe = guest_sbi_probe_extension(SBI_EXT_PMU, &out_val); > > > + GUEST_ASSERT(probe && out_val == 1); > > > + > > > + if (get_host_sbi_spec_version() < sbi_mk_version(2, 0)) > > > + __GUEST_ASSERT(0, "SBI implementation version doesn't support PMU Snapshot"); > > > +} > > > > It's a pity we can't check the SBI spec version that KVM is advertising > > from KVM userspace. Normally we'd want to check something like this at > > the start of the test with TEST_REQUIRE() before running a VCPU in order > > to generate a skip exit. > > > > Agreed. I will send a separate series for that as it is an ABI change. > > > (We probably should allow reading and even writing the SBI spec version > > from the VMM in order to better support migration.) > > > > How that would work for SBI spec version write use case ? For migraiton, you > can't go back to older SBI versions in the host. Isn't it ? > > Considering this case your VM is running with PMU snapshot as the host has > SBI v2.0. It can't be migrated to v1.0 and expecting it work. Correct ? > We can start a VM on a host with SBI v2.0, but tell KVM to tell the VM that it has v1.0. Then, the guest shouldn't use any features from SBI that appear after v1.0 and it should be safe to migrate to a host with v1.0. A more likely scenario might be this though: 1. KVM userspace checks and captures the SBI version of the host where the VM is first being launched, e.g. v2.0 2. The VM gets migrated to another host which supports something later, e.g. v3.0, but to - avoid possibly confusing the guest we tell the destination host that it should expose v2.0 as the SBI version - allow rollback to the source host without concern that the guest has already seen v3.0 and started to use something that the source can't provide Thanks, drew
On 4/10/24 00:10, Andrew Jones wrote: > On Tue, Apr 09, 2024 at 03:52:40PM -0700, Atish Patra wrote: >> On 4/5/24 06:11, Andrew Jones wrote: >>> On Wed, Apr 03, 2024 at 01:04:50AM -0700, Atish Patra wrote: > ... >>>> + probe = guest_sbi_probe_extension(SBI_EXT_PMU, &out_val); >>>> + GUEST_ASSERT(probe && out_val == 1); >>>> + >>>> + if (get_host_sbi_spec_version() < sbi_mk_version(2, 0)) >>>> + __GUEST_ASSERT(0, "SBI implementation version doesn't support PMU Snapshot"); >>>> +} >>> It's a pity we can't check the SBI spec version that KVM is advertising >>> from KVM userspace. Normally we'd want to check something like this at >>> the start of the test with TEST_REQUIRE() before running a VCPU in order >>> to generate a skip exit. >>> >> Agreed. I will send a separate series for that as it is an ABI change. >> >>> (We probably should allow reading and even writing the SBI spec version >>> from the VMM in order to better support migration.) >>> >> How that would work for SBI spec version write use case ? For migraiton, you >> can't go back to older SBI versions in the host. Isn't it ? >> >> Considering this case your VM is running with PMU snapshot as the host has >> SBI v2.0. It can't be migrated to v1.0 and expecting it work. Correct ? >> > We can start a VM on a host with SBI v2.0, but tell KVM to tell the VM > that it has v1.0. Then, the guest shouldn't use any features from SBI > that appear after v1.0 and it should be safe to migrate to a host with > v1.0. That depends on when the VMM request to KVM to change the version. Most of SBI implementation checks the SBI version at the boot and enable/disable feature based on the SBI version available. If the SBI version supported by KVM changes to an older one, the calls from VM will fail unexpectedly. > A more likely scenario might be this though: > > 1. KVM userspace checks and captures the SBI version of the host where > the VM is first being launched, e.g. v2.0 > 2. The VM gets migrated to another host which supports something later, > e.g. v3.0, but to > - avoid possibly confusing the guest we tell the destination host > that it should expose v2.0 as the SBI version > - allow rollback to the source host without concern that the guest > has already seen v3.0 and started to use something that the > source can't provide This makes sense though. As per my understanding, we should not allow modifying the SBI version that is less that the version VM already boot with. However, we can allow modifying the SBI version that is higher or same as the VM booted with. I can't think of a use case for the higher version though. > Thanks, > drew
On Wed, Apr 10, 2024 at 12:28:08AM -0700, Atish Patra wrote: > > On 4/10/24 00:10, Andrew Jones wrote: > > On Tue, Apr 09, 2024 at 03:52:40PM -0700, Atish Patra wrote: > > > On 4/5/24 06:11, Andrew Jones wrote: > > > > On Wed, Apr 03, 2024 at 01:04:50AM -0700, Atish Patra wrote: > > ... > > > > > + probe = guest_sbi_probe_extension(SBI_EXT_PMU, &out_val); > > > > > + GUEST_ASSERT(probe && out_val == 1); > > > > > + > > > > > + if (get_host_sbi_spec_version() < sbi_mk_version(2, 0)) > > > > > + __GUEST_ASSERT(0, "SBI implementation version doesn't support PMU Snapshot"); > > > > > +} > > > > It's a pity we can't check the SBI spec version that KVM is advertising > > > > from KVM userspace. Normally we'd want to check something like this at > > > > the start of the test with TEST_REQUIRE() before running a VCPU in order > > > > to generate a skip exit. > > > > > > > Agreed. I will send a separate series for that as it is an ABI change. > > > > > > > (We probably should allow reading and even writing the SBI spec version > > > > from the VMM in order to better support migration.) > > > > > > > How that would work for SBI spec version write use case ? For migraiton, you > > > can't go back to older SBI versions in the host. Isn't it ? > > > > > > Considering this case your VM is running with PMU snapshot as the host has > > > SBI v2.0. It can't be migrated to v1.0 and expecting it work. Correct ? > > > > > We can start a VM on a host with SBI v2.0, but tell KVM to tell the VM > > that it has v1.0. Then, the guest shouldn't use any features from SBI > > that appear after v1.0 and it should be safe to migrate to a host with > > v1.0. > > That depends on when the VMM request to KVM to change the version. > Most of SBI implementation checks the SBI version at the boot and > enable/disable > feature based on the SBI version available. If the SBI version supported by > KVM changes > to an older one, the calls from VM will fail unexpectedly. We have to configure KVM's SBI version before the first run of VCPUs, just like we should make sure ISA/SBI extensions are configured first. > > > A more likely scenario might be this though: > > > > 1. KVM userspace checks and captures the SBI version of the host where > > the VM is first being launched, e.g. v2.0 > > 2. The VM gets migrated to another host which supports something later, > > e.g. v3.0, but to > > - avoid possibly confusing the guest we tell the destination host > > that it should expose v2.0 as the SBI version > > - allow rollback to the source host without concern that the guest > > has already seen v3.0 and started to use something that the > > source can't provide > > This makes sense though. As per my understanding, we should not allow > modifying > the SBI version that is less that the version VM already boot with. > However, we can allow modifying the SBI version that is higher or same as > the VM booted with. Mostly only 'the same as'. Higher might work, but it's also risky since there could be guests out there which capture the version on boot and then for whatever reason do sanity checks against that later on and freak out when there's a change, even if the change went higher. > > I can't think of a use case for the higher version though. Maybe only for a coordinated update which uses kexec rather than a full shutdown+boot cycle, but I'm reaching... Regarding a full shutdown+boot cycle, in those cases, we're usually free to make changes as that's the same as a host kernel being shutdown and then being boot again after a firmware update. Thanks, drew
diff --git a/tools/testing/selftests/kvm/include/riscv/sbi.h b/tools/testing/selftests/kvm/include/riscv/sbi.h index 6675ca673c77..8c98bd99d450 100644 --- a/tools/testing/selftests/kvm/include/riscv/sbi.h +++ b/tools/testing/selftests/kvm/include/riscv/sbi.h @@ -8,6 +8,12 @@ #ifndef SELFTEST_KVM_SBI_H #define SELFTEST_KVM_SBI_H +/* SBI spec version fields */ +#define SBI_SPEC_VERSION_DEFAULT 0x1 +#define SBI_SPEC_VERSION_MAJOR_SHIFT 24 +#define SBI_SPEC_VERSION_MAJOR_MASK 0x7f +#define SBI_SPEC_VERSION_MINOR_MASK 0xffffff + /* SBI return error codes */ #define SBI_SUCCESS 0 #define SBI_ERR_FAILURE -1 @@ -33,6 +39,9 @@ enum sbi_ext_id { }; enum sbi_ext_base_fid { + SBI_EXT_BASE_GET_SPEC_VERSION = 0, + SBI_EXT_BASE_GET_IMP_ID, + SBI_EXT_BASE_GET_IMP_VERSION, SBI_EXT_BASE_PROBE_EXT = 3, }; enum sbi_ext_pmu_fid { @@ -60,6 +69,12 @@ union sbi_pmu_ctr_info { }; }; +struct riscv_pmu_snapshot_data { + u64 ctr_overflow_mask; + u64 ctr_values[64]; + u64 reserved[447]; +}; + struct sbiret { long error; long value; @@ -113,4 +128,14 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0, bool guest_sbi_probe_extension(int extid, long *out_val); +/* Make SBI version */ +static inline unsigned long sbi_mk_version(unsigned long major, + unsigned long minor) +{ + return ((major & SBI_SPEC_VERSION_MAJOR_MASK) << + SBI_SPEC_VERSION_MAJOR_SHIFT) | minor; +} + +unsigned long get_host_sbi_spec_version(void); + #endif /* SELFTEST_KVM_SBI_H */ diff --git a/tools/testing/selftests/kvm/lib/riscv/processor.c b/tools/testing/selftests/kvm/lib/riscv/processor.c index e8211f5d6863..ccb35573749c 100644 --- a/tools/testing/selftests/kvm/lib/riscv/processor.c +++ b/tools/testing/selftests/kvm/lib/riscv/processor.c @@ -502,3 +502,15 @@ bool guest_sbi_probe_extension(int extid, long *out_val) return true; } + +unsigned long get_host_sbi_spec_version(void) +{ + struct sbiret ret; + + ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_SPEC_VERSION, 0, + 0, 0, 0, 0, 0); + + GUEST_ASSERT(!ret.error); + + return ret.value; +} diff --git a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c index 8e7c7a3172d8..7d195be5c3d9 100644 --- a/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c +++ b/tools/testing/selftests/kvm/riscv/sbi_pmu_test.c @@ -19,6 +19,11 @@ #define RISCV_MAX_PMU_COUNTERS 64 union sbi_pmu_ctr_info ctrinfo_arr[RISCV_MAX_PMU_COUNTERS]; +/* Snapshot shared memory data */ +#define PMU_SNAPSHOT_GPA_BASE BIT(30) +static void *snapshot_gva; +static vm_paddr_t snapshot_gpa; + /* Cache the available counters in a bitmask */ static unsigned long counter_mask_available; @@ -178,6 +183,32 @@ static unsigned long read_counter(int idx, union sbi_pmu_ctr_info ctrinfo) return counter_val; } +static inline void verify_sbi_requirement_assert(void) +{ + long out_val = 0; + bool probe; + + probe = guest_sbi_probe_extension(SBI_EXT_PMU, &out_val); + GUEST_ASSERT(probe && out_val == 1); + + if (get_host_sbi_spec_version() < sbi_mk_version(2, 0)) + __GUEST_ASSERT(0, "SBI implementation version doesn't support PMU Snapshot"); +} + +static void snapshot_set_shmem(vm_paddr_t gpa, unsigned long flags) +{ + unsigned long lo = (unsigned long)gpa; +#if __riscv_xlen == 32 + unsigned long hi = (unsigned long)(gpa >> 32); +#else + unsigned long hi = gpa == -1 ? -1 : 0; +#endif + struct sbiret ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_SNAPSHOT_SET_SHMEM, + lo, hi, flags, 0, 0, 0); + + GUEST_ASSERT(ret.value == 0 && ret.error == 0); +} + static void test_pmu_event(unsigned long event) { unsigned long counter; @@ -210,6 +241,41 @@ static void test_pmu_event(unsigned long event) stop_reset_counter(counter, 0); } +static void test_pmu_event_snapshot(unsigned long event) +{ + unsigned long counter; + unsigned long counter_value_pre, counter_value_post; + unsigned long counter_init_value = 100; + struct riscv_pmu_snapshot_data *snapshot_data = snapshot_gva; + + counter = get_counter_index(0, counter_mask_available, 0, event); + counter_value_pre = read_counter(counter, ctrinfo_arr[counter]); + + /* Do not set the initial value */ + start_counter(counter, 0, 0); + dummy_func_loop(10000); + stop_counter(counter, SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT); + + /* The counter value is updated w.r.t relative index of cbase */ + counter_value_post = READ_ONCE(snapshot_data->ctr_values[0]); + __GUEST_ASSERT(counter_value_post > counter_value_pre, + "counter_value_post %lx counter_value_pre %lx\n", + counter_value_post, counter_value_pre); + + /* Now set the initial value and compare */ + WRITE_ONCE(snapshot_data->ctr_values[0], counter_init_value); + start_counter(counter, SBI_PMU_START_FLAG_INIT_SNAPSHOT, 0); + dummy_func_loop(10000); + stop_counter(counter, SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT); + + counter_value_post = READ_ONCE(snapshot_data->ctr_values[0]); + __GUEST_ASSERT(counter_value_post > counter_init_value, + "counter_value_post %lx counter_init_value %lx for counter\n", + counter_value_post, counter_init_value); + + stop_reset_counter(counter, 0); +} + static void test_invalid_event(void) { struct sbiret ret; @@ -272,6 +338,34 @@ static void test_pmu_basic_sanity(void) GUEST_DONE(); } +static void test_pmu_events_snaphost(void) +{ + int num_counters = 0; + struct riscv_pmu_snapshot_data *snapshot_data = snapshot_gva; + int i; + + /* Verify presence of SBI PMU and minimum requrired SBI version */ + verify_sbi_requirement_assert(); + + snapshot_set_shmem(snapshot_gpa, 0); + + /* Get the counter details */ + num_counters = get_num_counters(); + update_counter_info(num_counters); + + /* Validate shared memory access */ + GUEST_ASSERT_EQ(READ_ONCE(snapshot_data->ctr_overflow_mask), 0); + for (i = 0; i < num_counters; i++) { + if (counter_mask_available & (BIT(i))) + GUEST_ASSERT_EQ(READ_ONCE(snapshot_data->ctr_values[i]), 0); + } + /* Only these two events are guranteed to be present */ + test_pmu_event_snapshot(SBI_PMU_HW_CPU_CYCLES); + test_pmu_event_snapshot(SBI_PMU_HW_INSTRUCTIONS); + + GUEST_DONE(); +} + static void run_vcpu(struct kvm_vcpu *vcpu) { struct ucall uc; @@ -328,13 +422,46 @@ static void test_vm_events_test(void *guest_code) test_vm_destroy(vm); } +static void test_vm_setup_snapshot_mem(struct kvm_vm *vm, struct kvm_vcpu *vcpu) +{ + /* PMU Snapshot requires single page only */ + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, PMU_SNAPSHOT_GPA_BASE, 1, 1, 0); + /* PMU_SNAPSHOT_GPA_BASE is identity mapped */ + virt_map(vm, PMU_SNAPSHOT_GPA_BASE, PMU_SNAPSHOT_GPA_BASE, 1); + + snapshot_gva = (void *)(PMU_SNAPSHOT_GPA_BASE); + snapshot_gpa = addr_gva2gpa(vcpu->vm, (vm_vaddr_t)snapshot_gva); + sync_global_to_guest(vcpu->vm, snapshot_gva); + sync_global_to_guest(vcpu->vm, snapshot_gpa); +} + +static void test_vm_events_snapshot_test(void *guest_code) +{ + struct kvm_vm *vm = NULL; + struct kvm_vcpu *vcpu; + + vm = vm_create_with_one_vcpu(&vcpu, guest_code); + __TEST_REQUIRE(__vcpu_has_sbi_ext(vcpu, KVM_RISCV_SBI_EXT_PMU), + "SBI PMU not available, skipping test"); + + test_vm_setup_snapshot_mem(vm, vcpu); + + run_vcpu(vcpu); + + test_vm_destroy(vm); +} + int main(void) { + pr_info("SBI PMU basic test : starting\n"); test_vm_basic_test(test_pmu_basic_sanity); pr_info("SBI PMU basic test : PASS\n"); test_vm_events_test(test_pmu_events); pr_info("SBI PMU event verification test : PASS\n"); + test_vm_events_snapshot_test(test_pmu_events_snaphost); + pr_info("SBI PMU event verification with snapshot test : PASS\n"); + return 0; }