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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3707 bytes --]

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.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]