If ARMv7 boots in HYP mode, switch to SVC32.
Submitted by: Wojciech Macek <wma@semihalf.com>
Obtained from: Semihalf
Details
Details
- Reviewers
zbb andrew • ian - Commits
- rS287127: Leave hypervisor mode upon startup on ARMv7
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
This seems reasonable, but I'm not knowledgeable enough about hyper mode to know if this is really good or not.
| sys/arm/arm/locore-v6.S | ||
|---|---|---|
| 87 ↗ | (On Diff #3708) | Do we want to note this fact in a variable that can be exported as a sysctl? Here and below? |
Comment Actions
What about when we're ready to do something with hyper mode? The Zen guys bug us every once in a while to check out their patches because they have Dom0 support for us now (not sure how complete it is), is this just going to make their lives even harder? (I guess if I had ever even glanced at their patches I might know the answer to that.)
| sys/arm/arm/locore-v6.S | ||
|---|---|---|
| 87 ↗ | (On Diff #3708) | Note what fact? That the loader launched us in hyper mode? I'm not sure what use that would have. |
| 88 ↗ | (On Diff #3708) | ERET loads CPSR from SPSR_hyp, how do we know what mode that is going to return to? It seems like this code should be setting up the value and doing msr spsr_hyp, rN. |
| sys/arm/arm/locore-v6.S | ||
|---|---|---|
| 88 ↗ | (On Diff #3744) | What is the value in spsr at this point? Previous debugging experience tells me it's random bits, at least on some cortex-a processors. I think the right sequence would be to just manipulate the state of cpsr that's already in r0 and save it as spsr. Except... once that occurred to me, it then popped into my head that we don't need this entire sequence (from #if through #endif), instead we should replace the very first instruction above with: cpsid ifa, #(PSR_SVC32_MODE) And the same in mpentry below. That should work on all armv6/v7 no matter what the entry-time processor mode is, right? |
Comment Actions
Replies from Wojciech Macek (who doesn't have direct write access here)
- The write barrier.
Yep, it might be not necessary. I was testing it on the revision which did not have br_prod_tail atomic. I've just removed it and run the test again, but it will take about 24h, cause the reproduction rate is really low.
- Read barrier
I wouldn't say that it prevents reordering here. It rather guarantees observability of the valid data. Im my scenario I'm running drdb_dequeue_sc/buf_ring_dequeue_sc few times in the loop. It's inline, so the compiler puts them inside the loop's body. I suspect, that the issue is possible only on cortex-a15, due to its long pipeline and advanced branch predictors. In normal case, where the ring is empty, the dequeue_sc returns NULL. But the core wants to optimize memmory accessess, so there is a chance that it preloads its readbuffers with a data that can be used in further code execution. The cons_head does not change within the function, so there is no reason why the core shouldn't early-load the br->br_ring[cons_head] pointer, which might be outdated. The core just has no knowledge that the data is valid only when prod_tail changes. Nevertheless, without rmb the buffer did not work properly - dequeue function returned some "old" mbuf which left there from the previous pass. This is basically how I found this bug.
| sys/arm/arm/locore-v6.S | ||
|---|---|---|
| 88 ↗ | (On Diff #3744) | Michal pointed out that cpsid can't be used to leave Hyp mode, only return-from-exception is allowed to transition hyp->other. So nevermind on the cpsid suggestion, but I still think constructing spsr from the hyp mode cpsr is the right thing to do. |
Comment Actions
You probably additionally should install a stub vector allowing to go back to HYP mode if necessary and deal with the timers. FYI, the corresponding Bitrig change is: https://github.com/Bluerise/bitrig/commit/e1f661110a9a2e38ae0c00c4e394194ee0491a78
Comment Actions
From W. Macek:
I don't like to mess with hyp vector here from two reasons. This patch is intended to be simple doing as few changes as it could. On the other hand, some uboots provide an interface for safe hw peripheral acces using smc instruction. I think that would require more talks to find a proper solution, which is not the part of the current fix.
| sys/arm/arm/locore-v6.S | ||
|---|---|---|
| 69 ↗ | (On Diff #3744) | This will not be pushed |
| sys/arm/include/asm.h | ||
|---|---|---|
| 232 ↗ | (On Diff #3856) | If clang doesn't support the eret and msr instructions we should file a bug upstream and assign it to Renato Golin. For binutils we can add them locally if needed as we won't be updating it to a any later versions. |
Comment Actions
It looks like there is support for eret in llvm http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20141201/246665.html. Can you test with this, if it works we can pull it into FreeBSD.
Comment Actions
This patch doesn't work (Toolchain will not compile).
I'm posting the patch as it is.
Comment Actions
Can you wait for a few days. The change is part of llvm 3.6 which should be imported soon.
We would still need to either stop supporting gcc on armv6 or fix binutils to also handle the instructions.
| sys/arm/include/asm.h | ||
|---|---|---|
| 232 ↗ | (On Diff #3856) | Personally I think the hack is acceptable as long as it is clearly a short-term workaround. But I would like to see a comment explaining why it's done like this and what steps need to be taken before it can be replaced with the proper instruction, ideally with links to patches, PRs etc. for those steps. Here we need the Clang 3.6 import and either a local patch to gas (or a deprecated binutils) |
| sys/arm/include/asm.h | ||
|---|---|---|
| 234 ↗ | (On Diff #4064) | This is not quite correct, we are still missing support in binutils so will also fail to build with gcc. |
Comment Actions
We now have clang 3.6 in the tree so you should be able to use the instructions directly.
Comment Actions
Do you have any concerns regarding these changes? If so, please, let me know.
There is one issue still not solved - current gcc version probably will not be able to handle that. Do we really care?
Comment Actions
This is
Yes. We still care. While it may not be for long, we still support building arm with gcc in the tree.
Comment Actions
This is a polished version of previous commit:
- the check for binutils is based on clang variable which is more reliable.
- code duplication removal
- ensuring that interrupts are disabled on mode change
| sys/arm/arm/locore-v6.S | ||
|---|---|---|
| 51 ↗ | (On Diff #7602) | Isn't this a binutils issue too? |
| 74 ↗ | (On Diff #7602) | This is a binutils issue, no? Not a compiler issue? |
| sys/arm/include/asm.h | ||
| 246–248 ↗ | (On Diff #7602) | It would be better to always use these #defines in the code, and have a work around for binutils / clang that doesn't support them instead. |
| sys/arm/arm/locore-v6.S | ||
|---|---|---|
| 51 ↗ | (On Diff #7602) | From what I've learned because of licensing issues there is no way gcc >= 4.9 will be available. The initial version was for people's internal use. Here we drop it. And we use llvm 3.6 and higher by default. The description is of little relevance to the code but gives hints for further works. |
| 74 ↗ | (On Diff #7602) | For clang we have a built-in solution. gcc < 4.9needs workaround, and because of the previous comment, it is not considered as a actual option. |
| sys/arm/include/asm.h | ||
| 246–248 ↗ | (On Diff #7602) | You mean moving the code closer to the macro? Clang supports it natively. |
| sys/arm/arm/locore-v6.S | ||
|---|---|---|
| 51 ↗ | (On Diff #7602) | What does gcc have to do with a .S file? You still haven't adequately answered that question. |
| 74 ↗ | (On Diff #7602) | We've typically used a macro to handle this sort of thing so as to not littler the code with #ifdefs. When support for the old stuff goes away, we expand the macro in the code that uses it. This code, and what I'm objecting to, does the opposite: it puts the #ifdefs inline. Just define the macros differently and use them here. |
| sys/arm/include/asm.h | ||
| 246–248 ↗ | (On Diff #7602) | Yes, I mean exactly that. The clang #define will be simpler. |
Comment Actions
This is yet another revision of this patch, after out-of-band discussion with Warner, so should be much closer to a final version.
Comment Actions
This fell off my todo list to review... Sorry.
This looks good apart from the weird #ifdef nesting you have there.
| sys/arm/arm/locore-v6.S | ||
|---|---|---|
| 70 ↗ | (On Diff #7879) | You can have ifdefs inside of #defines? I think it needs to be the other way around. |