Page MenuHomeFreeBSD

Fix wrong cpu0 identification
ClosedPublic

Authored by leandro.lupori_gmail.com on Apr 23 2018, 7:22 PM.

Details

Summary

chrp_cpuref_init() was relying on the boot strap processor to be
the first child of /cpus. That was not always the case, specially
on pseries with FDT.

This change uses the "reg" property of each CPU instead and also
adds several sanity checks to avoid unexpected behavior (maybe
too many panics?).

The main observed symptom was interrupts being missed by the main
processor, leading to timeouts and the kernel aborting the boot.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nwhitehorn accepted this revision.Apr 23 2018, 7:49 PM
This revision is now accepted and ready to land.Apr 23 2018, 7:49 PM
  • Made cpu discovery accept non-contiguous ids
This revision now requires review to proceed.Apr 24 2018, 9:12 PM

Made cpu discovery accept non-contiguous ids

Latest QEMU versions give different IDs for CPUs sometimes.
With KVM disabled and two cores, it was observed that now
CPUs are given IDs 0 and 8, instead of 0 and 1, probably
because now the unused 1-7 CPU threads are being considered
in the count.

To handle this, CPU enumeration code was changed to support
non-contiguous ids.
The resulting array is produced by sorting the ids and placing
cpurefs contiguously, in the same order as the ids.

Note that now it's possible to have 2 CPU's numbered #0 and #8.
If this is not desired, the last for loop may just assign sequential
values to cr_cpuid.

Is there a reason not to just adopt the PowerNV version of this code, which already does the right thing?

Is there a reason not to just adopt the PowerNV version of this code, which already does the right thing?

Right now the main problem with adopting the PowerNV version of this code is that QEMU raises an exception when code tries to read PIR.

Although PowerISA specifies PIR read as privileged and not hypervisor privileged, and this seems to be a QEMU bug, making this change would:

1- Make FreeBSD stop booting under QEMU without KVM, until QEMU is fixed, assuming this is a bug in it.
As it is today, unsetting usefdt or limiting the number of cores to 1 would make the system boot. This would not be the case anymore.

2- Solve the problem only on real hardware, or only when QEMU gets fixed.

We should fix that, too, though. This code assumes that we are booting on CPU 0 -- the same assumption could continue as a stopgap until we figure out a reliable way to evaluate the boot processor.

We should fix that, too, though. This code assumes that we are booting on CPU 0 -- the same assumption could continue as a stopgap until we figure out a reliable way to evaluate the boot processor.

Right.

Reviewing the PowerISA it is now clear to me that PIR reads should be allowed in privileged mode too.
Making a quick fix in QEMU, I was able to boot with PowerNV equivalent code.
I've also sent this fix to qemu-devel mail list.

I just think that it will take some time until this fix gets in the next QEMU stable version.
Couldn't we also add a temporary workaround to keep FreeBSD working under QEMU until it gets fixed?
I've checked that QEMU emulates book-e instructions even on POWER8/pseries, so the workaround can be implemented by using GPIR.

What would be the best way to include both the correct code and the QEMU specific workaround?

The CPU is stored in the device tree in one of two ways:

  1. As an ihandle "cpu" in /chosen
  2. The "fdtbootcpu" integer property from /chosen

It should be much more robust to get the boot CPU from one of those than from PIR. Can you try that?

Ok, good to know, let me try that.

  • Use /chosen/{cpu,fdtbootcpu} to detect boot CPU
nwhitehorn accepted this revision.May 7 2018, 7:52 PM

Looks good to me. I haven't tested it, of course -- is this working well for you?

This revision is now accepted and ready to land.May 7 2018, 7:52 PM

Yes, it is working fine for me.
I tried a couple different number of CPUs, with and without usefdt, and it all worked fine.
Although for me /chosen/cpu always seems to exist, so the /chosen/fdtbootcpu part wasn't tested.

This revision was automatically updated to reflect the committed changes.

We should do the same thing (check fdtbootcpu rather than PIR) on PowerNV too. Thanks for the work on this patch!