Page MenuHomeFreeBSD

Allow cpu-v6.h to work on Cortex-A8 with SMP
ClosedPublic

Authored by andrew on Sep 30 2016, 12:12 PM.

Details

Summary

Fix the static inline functions in cpu-v6.h to only use the IS variants
when SMP is enabled. This allow these to be used with Cortex-A8

Test Plan

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

andrew updated this revision to Diff 20863.Sep 30 2016, 12:12 PM
andrew retitled this revision from to Allow cpu-v6.h to work on Cortex-A8 with SMP.
andrew updated this object.
andrew edited the test plan for this revision. (Show Details)
andrew added a reviewer: ARM.
skra added a subscriber: skra.Sep 30 2016, 8:02 PM

I understand the reason for this change, however, do we really want to run SMP kernel on UP boards? Maybe, __ARM_ARCH >= 7 && defined SMP test is only some obscure way how to eliminate boards which have not implemented SMP variants of cp15 operations like cortex A8?

sys/arm/include/cpu-v6.h
429 ↗(On Diff #20863)

Could same solution be used like in tlb_flush_range() to not test mp_ncpus repeatedly in a loop?

484 ↗(On Diff #20863)

Same note like in icache_sync().

mmel added a subscriber: mmel.Oct 1 2016, 9:26 AM

I'm sorry but I don't think that this is right way.
From local point of view:

  • mp_ncpus is not right variable for testing presence of multiprocessing extension. I have system where first CPU core is used for FreeBSD, second one is dedicated for application computing (without kernel), but SMP coherency I still required. I think that MPIDR[31:30] register is much better for this job.
  • I really don’t want to slowdown each single kernel by numerous “if (SMP)”. If you want to go by this way, then please use proper #ifdefs.
  • and, as Svata said, inserting immutable ‘if’s into inner loop is not optimal.

From global point of view I think that single generic kernel (UP and SMP) is in ARM world pure nonsense.

  • the speed difference between for compiled with -march=armv7a and -mcpu=cortex-a15 is more over 10% for computing intensive user mode program (I have not benchmarks for kernel yet, sorry). Mainly because HW divider is not present in all Cortex cores and because dual issue instruction scheduling.
  • there are numerous other places that must be fixed – page table attributes, TTBR…

But what makes sense for me is to have a “generic” kernel for each particular CPU type (A5/A6/A7/A9/A15/A17) or, at least for each “feature compatible” pair (A15+A7, …)

Again, I'm sorry for this type of negativism...

manu added a subscriber: manu.Oct 2 2016, 9:53 AM

Having a GENERIC kernel would be easier for image and re@ work.
Once pkgbase is there nothing is preventing us to provide GENERIC-CORTEXA7, GENERIC-CORTEXA8 etc ...
But having all board booting from the same image (minus the U-Boot) part will be better for everyone.

skra added a comment.Oct 2 2016, 10:37 AM

Here, we face the fact that various Cortex-A arm cores have various features. And even if I understand the reason for GENERIC, I don't like penalization of either custom or native kernel configurations just because of it. So, here we would need three functions set - one for UP, one for SMP, and one for GENERIC.

And I would like to remind that GENERIC will always be optimized for the worst arm core supported by it. So, some features would be unavailable if run on better arm cores.

mmel added a comment.Oct 2 2016, 2:58 PM
In D8092#167970, @manu wrote:

Having a GENERIC kernel would be easier for image and re@ work.

I understand. But from user point of view we gets:

  1. Longer boot times (because size of kernel image)
  2. Suboptimal GENERIC kernel performance.
  3. Slowdown of each specialized kernel.

I'm not able to understand motivation for 1) and 2) but, surely, I can live with this.
But I'm not able to accept 3), in any case.

Once pkgbase is there nothing is preventing us to provide GENERIC-CORTEXA7, GENERIC-CORTEXA8 etc ...
But having all board booting from the same image (minus the U-Boot) part will be better for everyone.

It's nice idea, but I'm not sure if this is possible at all.

andrew updated this revision to Diff 20956.Oct 3 2016, 10:52 AM

Use the mpide multiprocess extensions bit to check when to use them

skra added a comment.Oct 3 2016, 3:24 PM

I suggest to create ARM_GENERIC_KERNEL option which could be used in situation like here when something bizarre is implemented.

It brings two things in this case.
(1) Nobody won't wonder why it's needed to test mp extension feature again and again in SMP case.
(2) The mp extension feature test will apply only if GENERIC kernel is used. And it will be clear.

It makes code clean, self-explanatory, and useless things won't be done in case of not-GENERIC kernels.

sys/arm/arm/cpuinfo.c
135 ↗(On Diff #20956)

KASSERT(cpuinfo.mp_ext != 0, ...) if SMP is defined but not ARM_GENERIC_KERNEL.

sys/arm/include/cpu-v6.h
347 ↗(On Diff #20956)

I think that SMP is not supported for __ARM_ARCH == 6 as hardware TLB maintenance broadcasting is needed. If so, CTASSERT(__ARM_ARCH >= 7, ...) if SMP is defined - with explaining comment. Here, test SMP only.

This __ARM_ARCH >= 7 && defined SMP test may be used in more places, so if I'm correct, it may be replaced by defined SMP everywhere then.

352 ↗(On Diff #20956)

Here and on other places below, test cpuinfo.mp_ext only if ARM_GENERIC_KERNEL is defined.

You're confusing two things: the SMP option, and the MP extensions. The former just tells the kernel it should run on the all CPUs it can (with a few restrictions), the latter is the HW support needed for running on multiple CPUs.

There is no reason to artificially restrict running an SMP kernel on a Cortex-A8 just because it doesn't have the MP extensions, just as we should be able to run a non-SMP kernel on later hardware with the MP extensions.

I have created D8138 to start to split these out from each other, but it still needs us to fix any incorrect checks on SMP.

andrew updated this revision to Diff 21024.Oct 4 2016, 1:29 PM

Use CPU_CORTEXA8 to determine when to check for the MP extensions

skra added a comment.Oct 4 2016, 7:14 PM

You're confusing two things: the SMP option, and the MP extensions. The former just tells the kernel it should run on the all CPUs it can (with a few restrictions), the latter is the HW support needed for running on multiple CPUs.
There is no reason to artificially restrict running an SMP kernel on a Cortex-A8 just because it doesn't have the MP extensions, just as we should be able to run a non-SMP kernel on later hardware with the MP extensions.

I was just strict about that. However, you are right. It should be rather a warning (not KASSERT) that the kernel is built with SMP option but run on HW without MP extension. Note that I suggested to exclude GENERIC kernel from that check. Nevertheless, it's not related to your change. It was noted just as an extra idea.

I have created D8138 to start to split these out from each other, but it still needs us to fix any incorrect checks on SMP.

I'm glad that you have adopted the idea that not-GENERIC kernels should not be penalized by GENERIC one. Even if you did it your way. Thank you.

However, your change looks a little complicated now, when the knowledge about MP extension is presented (CPU_CORTEXA_MP). May I suggest the following pattern which utilizes the knowledge precisely?

#ifdef  CPU_CORTEXA_MP
do A
#else
do B
#endif

I would change CPU_CORTEXA_MP to something more general, but it may be done later.

skra added a comment.Oct 4 2016, 7:44 PM

Well, the pattern should be the following to work with GENERIC kernel.

#ifdef CPU_CORTEXA_MP
#define ARM_HAVE_MP_EXTENSIONS 1
#else
#define ARM_HAVE_MP_EXTENSIONS 0
#endif



#if ARM_HAVE_MP_EXTENSIONS == 0
if (cpuinfo.mp_ext == 0)
    do not-MP-ops
else
#endif
    do MP-ops

Note that ARM_HAVE_MP_EXTENSIONS may be defined in kernel configurations.

sys/arm/include/cpu-v6.h
365 ↗(On Diff #21024)

How does it work in GENERIC kernel case?

skra added a comment.Oct 4 2016, 8:21 PM

Hmm, if ARM_HAVE_MP_EXTENSIONS will be a hint defined in kernel configuration file, then the pattern will be the following:

#ifndef ARM_HAVE_MP_EXTENSIONS
if (cpuinfo.mp_ext == 0)
    do not-MP-ops
else
#endif
    do MP-ops

Of course, some run-time check when ARM_HAVE_MP_EXTENSIONS is defined that the hint is correct would be nice in place where cpuinfo.mp_ext is set.

This revision was automatically updated to reflect the committed changes.