Re: [kvm-unit-tests PATCH v5 2/2] arm/psci: Add PSCI CPU_OFF test case

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Drew,

On Tue, Jan 31, 2023 at 12:45:49PM +0100, Andrew Jones wrote:
> On Tue, Jan 31, 2023 at 11:16:22AM +0000, Alexandru Elisei wrote:
> ...
> > > > Does that make sense? Should I add a comment to make it clear why cpu-off
> > > > is skipped when cpu-on fails?
> > > 
> > > I missed that cpu_on_success was initialized to true. Seeing that now, I
> > > understand how the only time it's false is if the cpu-on test failed. When
> > > I thought it was initialized to false it had two ways to be false, failure
> > > or skip. I think it's a bit confusing to set a 'success' variable to true
> > > when the test is skipped. Also, we can relax the condition as to whether
> > > or not we try cpu-off by simply checking that all cpus, other than cpu0,
> > > are in idle. How about
> > > 
> > >  if (ERRATA(6c7a5dce22b3))
> > >      report(psci_cpu_on_test(), "cpu-on");
> > >  else
> > >      report_skip("Skipping unsafe cpu-on test. Set ERRATA_6c7a5dce22b3=y to enable.");
> > > 
> > >  assert(!cpu_idle(0));
> > 
> > cpu0 is the boot CPU, I don't see how cpu0 can execute this line of code
> > and be in idle at the same time.
> 
> That's why it's an assert and not an if, i.e. it should never happen. It
> could happen if things are messed up in the lib code, a previous test
> mucked with cpu_idle_mask, or a previous test idled cpu0 and manipulated
> another cpu into executing this line.
> 
> > Unless this is done for documenting
> > purposes, to explain why we compare the number of cpus in idle to nr_cpus
> > -1 below.
> 
> Exactly, and furthermore that we expect the missing cpu to be cpu0.
> 
> > But I still find it confusing, especially considering (almost)
> > the same assert is in smp.c:
> > 
> > void on_cpu_async(int cpu, void (*func)(void *data), void *data)
> > {
> > 	[..]
> >         assert_msg(cpu != 0 || cpu0_calls_idle, "Waiting on CPU0, which is unlikely to idle. "
> >                                                 "If this is intended set cpu0_calls_idle=1");
> > 
> > I know, it's a different scenario, but the point I'm trying to make is that
> > kvm-unit-tests really doesn't expect cpu0 to be in idle. I would prefer not
> > to have the assert here.
> 
> asserts are for things we assume, but also want to ensure, as other code
> depends on the assumptions. Please keep the assert. It doesn't hurt :-)

I'm keeping the assert then :)

Thanks,
Alex

> 
> Thanks,
> drew