linux-kernel - Re: [PATCH v5 4/5] KVM: selftests: Add test for PSCI SYSTEM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zw6jXEWwdW3S5Y6c@linux.dev>
Date: Tue, 15 Oct 2024 10:16:12 -0700
From: Oliver Upton <oliver.upton@...ux.dev>
To: David Woodhouse <dwmw2@...radead.org>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Jonathan Corbet <corbet@....net>,
	Marc Zyngier <maz@...nel.org>, James Morse <james.morse@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Zenghui Yu <yuzenghui@...wei.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
	Shuah Khan <shuah@...nel.org>, kvm@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
	linux-pm@...r.kernel.org, linux-kselftest@...r.kernel.org,
	Francesco Lavra <francescolavra.fl@...il.com>,
	Miguel Luis <miguel.luis@...cle.com>
Subject: Re: [PATCH v5 4/5] KVM: selftests: Add test for PSCI SYSTEM_OFF2

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
>