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.
--
Thanks,
Oliver
