On Tue, Apr 9, 2024 at 1:01 AM Andrew Jones ajones@ventanamicro.com wrote:
On Mon, Apr 08, 2024 at 05:37:19PM -0700, Atish Patra wrote:
On 4/5/24 05:50, Andrew Jones wrote:
On Wed, Apr 03, 2024 at 01:04:49AM -0700, Atish Patra wrote: ...
+static void test_pmu_basic_sanity(void) +{
- long out_val = 0;
- bool probe;
- struct sbiret ret;
- int num_counters = 0, i;
- union sbi_pmu_ctr_info ctrinfo;
- probe = guest_sbi_probe_extension(SBI_EXT_PMU, &out_val);
- GUEST_ASSERT(probe && out_val == 1);
- num_counters = get_num_counters();
- for (i = 0; i < num_counters; i++) {
ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_GET_INFO, i,0, 0, 0, 0, 0);/* There can be gaps in logical counter indicies*/if (ret.error)continue;GUEST_ASSERT_NE(ret.value, 0);ctrinfo.value = ret.value;/*** Accesibillity check of hardware and read capability of firmware counters.Accessibility
Fixed it.
* The spec doesn't mandate any initial value. No need to check any value.*/read_counter(i, ctrinfo);- }
- GUEST_DONE();
+}
+static void run_vcpu(struct kvm_vcpu *vcpu) +{
- struct ucall uc;
- vcpu_run(vcpu);
- switch (get_ucall(vcpu, &uc)) {
- case UCALL_ABORT:
REPORT_GUEST_ASSERT(uc);break;- case UCALL_DONE:
- case UCALL_SYNC:
break;- default:
TEST_FAIL("Unknown ucall %lu", uc.cmd);break;- }
+}
+void test_vm_destroy(struct kvm_vm *vm) +{
- memset(ctrinfo_arr, 0, sizeof(union sbi_pmu_ctr_info) * RISCV_MAX_PMU_COUNTERS);
- counter_mask_available = 0;
- kvm_vm_free(vm);
+}
+static void test_vm_basic_test(void *guest_code) +{
- struct kvm_vm *vm;
- 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");- vm_init_vector_tables(vm);
- /* Illegal instruction handler is required to verify read access without configuration */
- vm_install_exception_handler(vm, EXC_INST_ILLEGAL, guest_illegal_exception_handler);
I still don't see where the "verify" part is. The handler doesn't record that it had to handle anything.
The objective of the test is to ensure that we get an illegal instruction without configuration.
This part I guessed.
The presence of the registered exception handler is sufficient for that.
This part I disagree with. The handler may not be necessary and not run if we don't get the ILL. Usually when I write tests like these I set a boolean in the handler and check it after the instruction which should have sent us there to make sure we did indeed go there.
Ahh I got your point now. That makes sense. Since it was just a sanity test, I hadn't put the boolean check earlier. But you are correct about bugs in kvm code which wouldn't generate an expected ILL .
I have added it. Thanks for the suggestion :)
The verify part is that the test doesn't end up in a illegal instruction exception when you try to access a counter without configuring.
Let me know if you think we should more verbose comment to explain the scenario.
With a boolean the test code will be mostly self documenting, but a short comment saying why we expect the boolean to be set would be good too.
Thanks, drew
- vcpu_init_vector_tables(vcpu);
- run_vcpu(vcpu);
- test_vm_destroy(vm);
+}
+static void test_vm_events_test(void *guest_code) +{
- struct kvm_vm *vm = NULL;
- struct kvm_vcpu *vcpu = NULL;
- 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");- run_vcpu(vcpu);
- test_vm_destroy(vm);
+}
+int main(void) +{
- 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");
- return 0;
+}
2.34.1
Thanks, drew