Page MenuHomeFreeBSD

Release memory for CPUs that fail to init on ARM64
ClosedPublic

Authored by zbb on Aug 4 2015, 7:31 AM.

Details

Summary
cpu_init_fdt will now release memory allocated for structures serving
CPUs that have failed to init.

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

wma_semihalf.com retitled this revision from to Add memory for CPUs that fail to init on ARM64.
wma_semihalf.com updated this object.
wma_semihalf.com edited the test plan for this revision. (Show Details)
wma_semihalf.com added a reviewer: arm64.
wma_semihalf.com set the repository for this revision to rS FreeBSD src repository.
wma_semihalf.com retitled this revision from Add memory for CPUs that fail to init on ARM64 to Release memory for CPUs that fail to init on ARM64.Aug 4 2015, 7:31 AM

Seems fine - did you actually encounter CPUs failing to start, or just found by code inspection?

We found some troubles with CPU starting on one of our non-cavm arm64 boards. If you have no objections I'd like to submit this patch tomorrow.

This should only be the case when there is broken firmware, an incorrect dtb, or missing psci. In the latter we could check if psci is enabled in the dtb before attempting to start secondary cores, otherwise I feel the correct response is to panic.

zbb commandeered this revision.Aug 19 2015, 12:21 PM
zbb updated this revision to Diff 8057.
zbb added a reviewer: wma_semihalf.com.
zbb removed rS FreeBSD src repository as the repository for this revision.
zbb added a comment.Aug 19 2015, 12:25 PM
In D3297#69573, @andrew wrote:

This should only be the case when there is broken firmware, an incorrect dtb, or missing psci. In the latter we could check if psci is enabled in the dtb before attempting to start secondary cores, otherwise I feel the correct response is to panic.

Let's split the difference and put KASSERT in there that will panic when INVARIANTS are enabled. We assume that in most cases if CPU fails to start it is not harmful for the system (by that I mean that CPU is in power-off or etc.). What do you think about that?

In D3297#69857, @zbb wrote:

Let's split the difference and put KASSERT in there that will panic when INVARIANTS are enabled. We assume that in most cases if CPU fails to start it is not harmful for the system (by that I mean that CPU is in power-off or etc.). What do you think about that?

I'm not sure how useful that is. In what cases should failing to start a core not be fatal? All the cases I can think of I would want the kernel to fail as it indicates a larger problem.

zbb added a comment.Aug 19 2015, 5:08 PM
In D3297#69900, @andrew wrote:
In D3297#69857, @zbb wrote:

Let's split the difference and put KASSERT in there that will panic when INVARIANTS are enabled. We assume that in most cases if CPU fails to start it is not harmful for the system (by that I mean that CPU is in power-off or etc.). What do you think about that?

I'm not sure how useful that is. In what cases should failing to start a core not be fatal? All the cases I can think of I would want the kernel to fail as it indicates a larger problem.

Depends on CPU wake-up method. If we just release the CPU core and it does not notify his presence it will probably be fatal for the system. If we have a firmware that wakes CPUs and we send a request to our monitor (or whatever) and it fails to start the CPU (because it limits the number of CPUs or has some arbitrary settings ), we end up having 1 CPU less in our system.
Still it's a political dispute and we are not going to oppose if you really prefer good old panic there.

This revision was automatically updated to reflect the committed changes.