diff mbox series

[v5,4/5] KVM: selftests: Add test for PSCI SYSTEM_OFF2

Message ID 20240926184546.833516-5-dwmw2@infradead.org
State New
Headers show
Series Add PSCI v1.3 SYSTEM_OFF2 support for hibernation | expand

Commit Message

David Woodhouse Sept. 26, 2024, 6:37 p.m. UTC 
From: David Woodhouse <dwmw@amazon.co.uk>

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 .../testing/selftests/kvm/aarch64/psci_test.c | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)

Comments

Oliver Upton Oct. 1, 2024, 3:33 p.m. UTC | #1 
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.
David Woodhouse Oct. 12, 2024, 9:28 a.m. UTC | #2 
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.
Sean Christopherson Oct. 17, 2024, 12:23 a.m. UTC | #3 
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
David Woodhouse Oct. 19, 2024, 5:14 p.m. UTC | #4 
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 mbox series

Patch

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;
 }