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 :-)
Thanks,
drew
