| Message ID | 20240926184546.833516-5-dwmw2@infradead.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Add PSCI v1.3 SYSTEM_OFF2 support for hibernation | expand |
On Thu, Sep 26, 2024 at 07:37:59PM +0100, David Woodhouse wrote: > +static void guest_test_system_off2(void) > +{ > + uint64_t ret; > + > + /* assert that SYSTEM_OFF2 is discoverable */ > + GUEST_ASSERT(psci_features(PSCI_1_3_FN_SYSTEM_OFF2) & > + BIT(PSCI_1_3_HIBERNATE_TYPE_OFF)); > + GUEST_ASSERT(psci_features(PSCI_1_3_FN64_SYSTEM_OFF2) & > + BIT(PSCI_1_3_HIBERNATE_TYPE_OFF)); > + Can you also assert that the guest gets INVALID_PARAMETERS if it sets arg1 or arg2 to a reserved value? > + ret = psci_system_off2(PSCI_1_3_HIBERNATE_TYPE_OFF); > + GUEST_SYNC(ret); > +} > + > +static void host_test_system_off2(void) > +{ > + struct kvm_vcpu *source, *target; > + uint64_t psci_version = 0; > + struct kvm_run *run; > + struct kvm_vm *vm; > + > + vm = setup_vm(guest_test_system_off2, &source, &target); > + vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version); > + TEST_ASSERT(psci_version >= PSCI_VERSION(0, 2), > + "Unexpected PSCI version %lu.%lu", > + PSCI_VERSION_MAJOR(psci_version), > + PSCI_VERSION_MINOR(psci_version)); > + > + if (psci_version < PSCI_VERSION(1,3)) > + goto skip; I'm not following this. Is there a particular reason why we'd want to skip for v1.2 and fail the test for anything less than that? Just do TEST_REQUIRE(psci_version >= PSCI_VERSION(1, 3)), it makes the requirements obvious in the case someone runs new selftests on an old kernel.
On Tue, 2024-10-01 at 08:33 -0700, Oliver Upton wrote: > On Thu, Sep 26, 2024 at 07:37:59PM +0100, David Woodhouse wrote: > > +static void guest_test_system_off2(void) > > +{ > > + uint64_t ret; > > + > > + /* assert that SYSTEM_OFF2 is discoverable */ > > + GUEST_ASSERT(psci_features(PSCI_1_3_FN_SYSTEM_OFF2) & > > + BIT(PSCI_1_3_HIBERNATE_TYPE_OFF)); > > + GUEST_ASSERT(psci_features(PSCI_1_3_FN64_SYSTEM_OFF2) & > > + BIT(PSCI_1_3_HIBERNATE_TYPE_OFF)); > > + > > Can you also assert that the guest gets INVALID_PARAMETERS if it sets > arg1 or arg2 to a reserved value? Ack (having actually made KVM do so, as you noted on a previous patch). > > + ret = psci_system_off2(PSCI_1_3_HIBERNATE_TYPE_OFF); > > + GUEST_SYNC(ret); > > +} > > + > > +static void host_test_system_off2(void) > > +{ > > + struct kvm_vcpu *source, *target; > > + uint64_t psci_version = 0; > > + struct kvm_run *run; > > + struct kvm_vm *vm; > > + > > + vm = setup_vm(guest_test_system_off2, &source, &target); > > + vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version); > > + TEST_ASSERT(psci_version >= PSCI_VERSION(0, 2), > > + "Unexpected PSCI version %lu.%lu", > > + PSCI_VERSION_MAJOR(psci_version), > > + PSCI_VERSION_MINOR(psci_version)); > > + > > + if (psci_version < PSCI_VERSION(1,3)) > > + goto skip; > > I'm not following this. Is there a particular reason why we'd want to > skip for v1.2 and fail the test for anything less than that? These tests unconditionally set KVM_ARM_VCPU_PSCI_0_2 in setup_vm(). Which is probably OK assuming support for that that predates KVM_CAP_ARM_SYSTEM_SUSPEND (which is already a TEST_REQUIRE() right at the start). So the world is very broken if KVM actually starts a VM but the version isn't at least 0.2, and it seemed like it warranted an actual failure. > Just do TEST_REQUIRE(psci_version >= PSCI_VERSION(1, 3)), it makes the > requirements obvious in the case someone runs new selftests on an old > kernel. I don't think we want to put that in main() and skip the other checks that would run on earlier kernels. (Even if we had easy access to psci_version without actually running a test and starting a VM). I could put it into host_test_system_off2() which runs last (and comment the invocations in main() to say that they're in increasing order of PSCI version) to accommodate such). But then it seems that I'd be the target of this comment in ksft_exit_skip()... /* * FIXME: several tests misuse ksft_exit_skip so produce * something sensible if some tests have already been run * or a plan has been printed. Those tests should use * ksft_test_result_skip or ksft_exit_fail_msg instead. */ I suspect the real answer here is that the individual tests here be calling ksft_test_result_pass(), and the system_off2 one should call ksft_test_result_skip() if it skips? That's probably material for a completely separate patch series, but it seems like we're better off leaving host_test_system_off2() as I have it here, so that it's just a case of adding that call before the 'goto skip'. I'll add an explicit comment about the 0.2 check though, saying that it should never happen so we might as well have the ASSERT for it.
On Sat, Oct 12, 2024 at 10:28:10AM +0100, David Woodhouse wrote: > On Tue, 2024-10-01 at 08:33 -0700, Oliver Upton wrote: > > On Thu, Sep 26, 2024 at 07:37:59PM +0100, David Woodhouse wrote: > > > + vm = setup_vm(guest_test_system_off2, &source, &target); > > > + vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version); > > > + TEST_ASSERT(psci_version >= PSCI_VERSION(0, 2), > > > + "Unexpected PSCI version %lu.%lu", > > > + PSCI_VERSION_MAJOR(psci_version), > > > + PSCI_VERSION_MINOR(psci_version)); > > > + > > > + if (psci_version < PSCI_VERSION(1,3)) > > > + goto skip; > > > > I'm not following this. Is there a particular reason why we'd want to > > skip for v1.2 and fail the test for anything less than that? > > These tests unconditionally set KVM_ARM_VCPU_PSCI_0_2 in setup_vm(). > Which is probably OK assuming support for that that predates > KVM_CAP_ARM_SYSTEM_SUSPEND (which is already a TEST_REQUIRE() right at > the start). > > So the world is very broken if KVM actually starts a VM but the version > isn't at least 0.2, and it seemed like it warranted an actual failure. If we're looking at this from a testing lens then KVM coming up with any PSCI version other than KVM_ARM_PSCI_LATEST (i.e. v1.3) is a bug. So maybe we can tighten that assertion because... > > Just do TEST_REQUIRE(psci_version >= PSCI_VERSION(1, 3)), it makes the > > requirements obvious in the case someone runs new selftests on an old > > kernel. > > I don't think we want to put that in main() and skip the other checks > that would run on earlier kernels. Running KVM selftests on older kernels in a meaningful way isn't something we support. At all. An example of this is commit 8a53e1302133 ("KVM: selftests: Require KVM_CAP_USER_MEMORY2 for tests that create memslots"), which skips ~everything for kernels older than 6.8. > (Even if we had easy access to > psci_version without actually running a test and starting a VM). > > I could put it into host_test_system_off2() which runs last (and > comment the invocations in main() to say that they're in increasing > order of PSCI version) to accommodate such). But then it seems that I'd > be the target of this comment in ksft_exit_skip()... > > /* > * FIXME: several tests misuse ksft_exit_skip so produce > * something sensible if some tests have already been run > * or a plan has been printed. Those tests should use > * ksft_test_result_skip or ksft_exit_fail_msg instead. > */ > > I suspect the real answer here is that the individual tests here be > calling ksft_test_result_pass(), and the system_off2 one should call > ksft_test_result_skip() if it skips? modulo a few one-offs, KVM selftests doesn't use the kselftest harness so it isn't subject to this comment. Since there's no test plan, we can skip at any time. > I'll add an explicit comment about the 0.2 check though, saying that it > should never happen so we might as well have the ASSERT for it. After looking at this again, I think we should do one of the following: - TEST_REQUIRE() that the PSCI version is at least v1.3, making the dependency clear on older kernels. - TEST_REQUIRE() for v1.3, which would provide better test coverage on upstream.
On Tue, Oct 15, 2024 at 09:05:18AM -0700, Oliver Upton wrote: > On Sat, Oct 12, 2024 at 10:28:10AM +0100, David Woodhouse wrote: > > On Tue, 2024-10-01 at 08:33 -0700, Oliver Upton wrote: > > > On Thu, Sep 26, 2024 at 07:37:59PM +0100, David Woodhouse wrote: > > > > + vm = setup_vm(guest_test_system_off2, &source, &target); > > > > + vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version); > > > > + TEST_ASSERT(psci_version >= PSCI_VERSION(0, 2), > > > > + "Unexpected PSCI version %lu.%lu", > > > > + PSCI_VERSION_MAJOR(psci_version), > > > > + PSCI_VERSION_MINOR(psci_version)); > > > > + > > > > + if (psci_version < PSCI_VERSION(1,3)) > > > > + goto skip; > > > > > > I'm not following this. Is there a particular reason why we'd want to > > > skip for v1.2 and fail the test for anything less than that? > > > > These tests unconditionally set KVM_ARM_VCPU_PSCI_0_2 in setup_vm(). > > Which is probably OK assuming support for that that predates > > KVM_CAP_ARM_SYSTEM_SUSPEND (which is already a TEST_REQUIRE() right at > > the start). > > > > So the world is very broken if KVM actually starts a VM but the version > > isn't at least 0.2, and it seemed like it warranted an actual failure. > > If we're looking at this from a testing lens then KVM coming up with any > PSCI version other than KVM_ARM_PSCI_LATEST (i.e. v1.3) is a bug. So > maybe we can tighten that assertion because... > > > > Just do TEST_REQUIRE(psci_version >= PSCI_VERSION(1, 3)), it makes the > > > requirements obvious in the case someone runs new selftests on an old > > > kernel. > > > > I don't think we want to put that in main() and skip the other checks > > that would run on earlier kernels. > > Running KVM selftests on older kernels in a meaningful way isn't > something we support. At all. An example of this is commit > 8a53e1302133 ("KVM: selftests: Require KVM_CAP_USER_MEMORY2 for > tests that create memslots"), which skips ~everything for kernels older > than 6.8. > > > (Even if we had easy access to > > psci_version without actually running a test and starting a VM). > > > > I could put it into host_test_system_off2() which runs last (and > > comment the invocations in main() to say that they're in increasing > > order of PSCI version) to accommodate such). But then it seems that I'd > > be the target of this comment in ksft_exit_skip()... > > > > /* > > * FIXME: several tests misuse ksft_exit_skip so produce > > * something sensible if some tests have already been run > > * or a plan has been printed. Those tests should use > > * ksft_test_result_skip or ksft_exit_fail_msg instead. > > */ > > > > I suspect the real answer here is that the individual tests here be > > calling ksft_test_result_pass(), and the system_off2 one should call > > ksft_test_result_skip() if it skips? > > modulo a few one-offs, KVM selftests doesn't use the kselftest harness > so it isn't subject to this comment. Since there's no test plan, we can > skip at any time. > > > I'll add an explicit comment about the 0.2 check though, saying that it > > should never happen so we might as well have the ASSERT for it. > > After looking at this again, I think we should do one of the following: > > - TEST_REQUIRE() that the PSCI version is at least v1.3, making the > dependency clear on older kernels. > > - TEST_REQUIRE() for v1.3, which would provide better test coverage on > upstream. Sorry, I meant TEST_ASSERT() here. > -- > Thanks, > Oliver >
On Tue, Oct 15, 2024, Oliver Upton wrote: > On Sat, Oct 12, 2024 at 10:28:10AM +0100, David Woodhouse wrote: > > I suspect the real answer here is that the individual tests here be > > calling ksft_test_result_pass(), and the system_off2 one should call > > ksft_test_result_skip() if it skips? > > modulo a few one-offs, KVM selftests doesn't use the kselftest harness > so it isn't subject to this comment. Since there's no test plan, we can > skip at any time. FWIW, I have some ideas on how to use the nicer pieces of kselftest harness, if anyone is looking for a project. :-) https://lore.kernel.org/all/ZjUwqEXPA5QVItyX@google.com
On Tue, 2024-10-15 at 10:16 -0700, Oliver Upton wrote: > > > After looking at this again, I think we should do one of the following: > > > > - TEST_REQUIRE() that the PSCI version is at least v1.3, making the > > dependency clear on older kernels. > > > > - TEST_REQUIRE() for v1.3, which would provide better test coverage on > > upstream. > > Sorry, I meant TEST_ASSERT() here. Ack. I'll do the latter.
diff --git a/tools/testing/selftests/kvm/aarch64/psci_test.c b/tools/testing/selftests/kvm/aarch64/psci_test.c index 61731a950def..b7e37956aecf 100644 --- a/tools/testing/selftests/kvm/aarch64/psci_test.c +++ b/tools/testing/selftests/kvm/aarch64/psci_test.c @@ -54,6 +54,15 @@ static uint64_t psci_system_suspend(uint64_t entry_addr, uint64_t context_id) return res.a0; } +static uint64_t psci_system_off2(uint64_t type) +{ + struct arm_smccc_res res; + + smccc_hvc(PSCI_1_3_FN64_SYSTEM_OFF2, type, 0, 0, 0, 0, 0, 0, &res); + + return res.a0; +} + static uint64_t psci_features(uint32_t func_id) { struct arm_smccc_res res; @@ -188,11 +197,63 @@ static void host_test_system_suspend(void) kvm_vm_free(vm); } +static void guest_test_system_off2(void) +{ + uint64_t ret; + + /* assert that SYSTEM_OFF2 is discoverable */ + GUEST_ASSERT(psci_features(PSCI_1_3_FN_SYSTEM_OFF2) & + BIT(PSCI_1_3_HIBERNATE_TYPE_OFF)); + GUEST_ASSERT(psci_features(PSCI_1_3_FN64_SYSTEM_OFF2) & + BIT(PSCI_1_3_HIBERNATE_TYPE_OFF)); + + ret = psci_system_off2(PSCI_1_3_HIBERNATE_TYPE_OFF); + GUEST_SYNC(ret); +} + +static void host_test_system_off2(void) +{ + struct kvm_vcpu *source, *target; + uint64_t psci_version = 0; + struct kvm_run *run; + struct kvm_vm *vm; + + vm = setup_vm(guest_test_system_off2, &source, &target); + vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version); + TEST_ASSERT(psci_version >= PSCI_VERSION(0, 2), + "Unexpected PSCI version %lu.%lu", + PSCI_VERSION_MAJOR(psci_version), + PSCI_VERSION_MINOR(psci_version)); + + if (psci_version < PSCI_VERSION(1,3)) + goto skip; + + vcpu_power_off(target); + run = source->run; + + enter_guest(source); + + TEST_ASSERT_KVM_EXIT_REASON(source, KVM_EXIT_SYSTEM_EVENT); + TEST_ASSERT(run->system_event.type == KVM_SYSTEM_EVENT_SHUTDOWN, + "Unhandled system event: %u (expected: %u)", + run->system_event.type, KVM_SYSTEM_EVENT_SHUTDOWN); + TEST_ASSERT(run->system_event.ndata >= 1, + "Unexpected amount of system event data: %u (expected, >= 1)", + run->system_event.ndata); + TEST_ASSERT(run->system_event.data[0] & KVM_SYSTEM_EVENT_SHUTDOWN_FLAG_PSCI_OFF2, + "PSCI_OFF2 flag not set. Flags %llu (expected %llu)", + run->system_event.data[0], KVM_SYSTEM_EVENT_SHUTDOWN_FLAG_PSCI_OFF2); + + skip: + kvm_vm_free(vm); +} + int main(void) { TEST_REQUIRE(kvm_has_cap(KVM_CAP_ARM_SYSTEM_SUSPEND)); host_test_cpu_on(); host_test_system_suspend(); + host_test_system_off2(); return 0; }