Bring Raspberry Pi 4 (RPi4) support #293
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lulu98
force-pushed
the
rpi4_porting
branch
4 times, most recently
from
680d8be
to
81f54fa
Compare
|
yyshen
reviewed
Jan 28, 2021
| @@ -23,6 +23,11 @@ elseif(KernelArmCortexA53) | |||
| elseif(KernelArmCortexA57) | |||
| set(KernelArmPASizeBits44 ON) | |||
| math(EXPR KernelPaddrUserTop "(1 << 44) - 1") | |||
| elseif(KernelArmCortexA72) | |||
| # For Cortex-A72 in AArch64 state, the physical address range is 44 bits | |||
| # (https://developer.arm.com/documentation/100095/0001/memory-management-unit/about-the-mmu) | |||
There was a problem hiding this comment.
maybe put the link to the docs to the commit messages instead of here?
There was a problem hiding this comment.
Actually, we have put them here on purpose, because in the end you are looking at the source and not at the commit message (or the commit/blame log) when you try to understand or debug something. We can also add it to the commit message, but I have some doubt about the practical value.
There was a problem hiding this comment.
The doc link here showed where you get the PA size. I agree docs in source code might help a lot, but this information is not very hard to get.
BTW, does the ID_AA64MMFR0_EL1 have 16T in the PARange field on real hardware (it probably should)? I know some customised SoCs cut on the actual PA size bits.
There was a problem hiding this comment.
So according to https://developer.arm.com/documentation/100095/0002/system-control/aarch64-register-descriptions/aarch64-memory-model-feature-register-0--el1, the PARange field "returns 0x4 to indicate that the processor supports a 44-bit physical address range, that is, 16TByte". I am not sure what is returned on the actual hardware though.
| ) | ||
| # According to https://developer.arm.com/documentation/100095/0001/functional-description/about-the-cortex-a72-processor-functions/components-of-the-processor |
There was a problem hiding this comment.
I'd prefer to keep the URL in the source, as this is what most people in the future will be looking at primarily.
There was a problem hiding this comment.
I think this comment should be in a place more specific to the a72 processor? This comment is in a block that applies to 6 different processor types, but is only specific to the a72 which reduces its relevance. Arguably, it's getting to the point where having micro-arch specific config spread around many config files makes it hard to tell if everything is correctly added for a new micro-arch.
There was a problem hiding this comment.
Let's postpone this as a refactoring step after merging this.
src/plat/bcm2711/config.cmake
Outdated
| elseif("${KernelSel4Arch}" STREQUAL aarch64) | ||
| declare_seL4_arch(aarch64) | ||
| else() | ||
| fallback_declare_seL4_arch_default(aarch32) |
There was a problem hiding this comment.
default as aarch64 since it is armv8?
There was a problem hiding this comment.
It makes sense to make AARCH64 the standard now cut some legacy. We were just unsure if anybody still has some strings attached to AARCH32 from RasPi3, so we put this in there. But I'm more than happy to remove this and define that AARCH32 must be explicitly requested.
There was a problem hiding this comment.
The RPi4 port was based on the RPi3 port. In https://github.com/seL4/seL4/blob/master/src/plat/bcm2837/config.cmake, the default architecture is also aarch32 although the cortex-a53 is ARMv8.
There was a problem hiding this comment.
I think we should ask if aarch64 might be more appropriate as the default for armv8 chips. RPi3 being aarch32 by default does not mean RPi4 should do the same. I know this is a bit nitpicking, so it is up to you :-)
|
|
||
| memory@0 { | ||
| /* This is configurable in the Pi's config.txt, but we use 128MiB of RAM by default. */ | ||
| reg = <0x00 0x00000000 0x08000000>; |
There was a problem hiding this comment.
what is the memory size on the device? why is 128MiB picked?
There was a problem hiding this comment.
The RPi4 (bcm2711) port is generally based on the RPi3 (bcm2837) port. In the corresponding overlay-rpi3.dts file (https://github.com/seL4/seL4/blob/master/src/plat/bcm2837/overlay-rpi3.dts), also only 128MiB are declared although the RPi3 has more than 128 MiB RAM. Furthermore, the RPi4 comes with different RAM sizes (1,2,4,8 GiB) (https://www.canadarobotix.com/blogs/how-to/how-to-tell-which-raspberry-pi-4-ram-size-do-i-have). I mean, it would be possible to create a different overlay.dts file for the different RPi4 versions. But in the end anyone who wants a different RAM size can modify this file to his needs anyways and the current version seems to work just fine for now.
There was a problem hiding this comment.
Thanks for the explanation. So the minimal size for all RPi4 boards is 1GiB, which could be used here instead of 128. You mentioned " This is configurable in the Pi's config.txt". What is the config.txt file? Modifying the file can change the usable memory size?
There was a problem hiding this comment.
The comment also stems from the corresponding RPi3 file. The config.txt file is part of the necessary boot files for Raspis (https://www.raspberrypi.org/documentation/configuration/boot_folder.md). In the config.txt, you can specify configuration parameters for setting up the Raspi. According to https://www.raspberrypi.org/documentation/configuration/config-txt/memory.md, you can specify the gpu_mem size (VideoCore size) and limit the total amount of RAM with total_mem. The minimum value here is 128MiB, which might be an explanation for this configuration.
| @@ -83,7 +88,8 @@ config_option( | |||
| KernelArmHypervisorSupport ARM_HYPERVISOR_SUPPORT | |||
| "Build as Hypervisor. Utilise ARM virtualisation extensions to build the kernel as a hypervisor" | |||
| DEFAULT ${default_hyp_support} | |||
| DEPENDS "KernelArmCortexA15 OR KernelArmCortexA35 OR KernelArmCortexA57 OR KernelArmCortexA53" | |||
| DEPENDS | |||
| "KernelArmCortexA15 OR KernelArmCortexA35 OR KernelArmCortexA57 OR KernelArmCortexA53 OR KernelArmCortexA72" | |||
There was a problem hiding this comment.
Have the hypervisor functions been tested on A72 (i.e., bringing up a Linux VM)?
There was a problem hiding this comment.
So far this has not been tested in detail. We are just assuming the A72 basically works like the other ARMv8 cores. Also, internally we have some ongoing internal activity about this, but that is still in the early stages. Maybe we should keep this out of the current commit for now. Is there any quick way to test this vie seL4test?
There was a problem hiding this comment.
I was imaging someone turns on this option on RPi4 but finds out that it does not work. It should be quite similar, but you never know until you actually run it. I suggest we keep this option off until we confirm that we can run a VM on it.
|
@ssrg-bamboo |
|
Hello, I'm a bot! I'll bring this PR into Trustworthy Systems and run some tests |
|
All the tests we ran have passed! Nice job! |
oliver-wm
approved these changes
Jan 29, 2021
axel-h
approved these changes
Feb 5, 2021
|
@ssrg-bamboo |
|
Hello, I'm a bot! I'll bring this PR into Trustworthy Systems and run some tests |
Test results for commit ffe69bbSummaryNOTE: some test results have been updated!
Detailed failure logs
Job: gcc-armv8a-hw-build-1 Job: clang-armv8a-hw-build-1 Job: gcc-armv8a-hw-build-2 This is the most I can report on right now, sorry! |
kent-mcleod
approved these changes
Feb 8, 2021
axel-h
approved these changes
Feb 8, 2021
|
@ssrg-bamboo |
|
Hello, I'm a bot! I'll bring this PR into Trustworthy Systems and run some tests |
|
Do you need to update this PR? |
|
There is #293 (comment) that we should address. It would be cleaner to do it now and not fix it later. I hope to catch @lulu98 to get this done quickly. |
This comment has been minimized.
This comment has been minimized.
axel-h
force-pushed
the
rpi4_porting
branch
2 times, most recently
from
310a818
to
3a31e95
Compare
|
addressed #293 (comment), please merge |
|
@ssrg-bamboo |
|
Hello, I'm a bot! I'll bring this PR into Trustworthy Systems and run some tests |
|
All the tests we ran have passed! Nice job! |
|
@ssrg-bamboo |
|
Will retry cooking up a new image to test tomorrow on our board. |
|
Hello, I'm a bot! I'll bring this PR into Trustworthy Systems and run some tests |
Test results for commit 3f72433SummaryNOTE: some test results have been updated!
Detailed failure logs
Job: ARMMCS This is the most I can report on right now, sorry! |
|
@ssrg-bamboo |
|
Hello, I'm a bot! I'll bring this PR into Trustworthy Systems and run some tests |
Test results for commit 5ab94eeSummaryNOTE: some test results have been updated!
Detailed failure logs
Job: gcc-rv64imac-hw-build This is the most I can report on right now, sorry! |
Signed-off-by: Lukas Graber <lukas.graber@hensoldt-cyber.de>
|
@ssrg-bamboo |
|
Hello, I'm a bot! I'll bring this PR into Trustworthy Systems and run some tests |
Test results for commit 3f08d92SummaryNOTE: some test results have been updated!
Detailed failure logs
Job: gcc-rv64imac-hw-build This is the most I can report on right now, sorry! |
Test results for commit 3f08d92SummaryNOTE: some test results have been updated!
Detailed failure logs
Job: gcc-rv64imac-hw-build This is the most I can report on right now, sorry! |
|
Is the problem on our side? I am not exactly sure what |
|
It's on our side. Will have it sorted soon. |
Test results for commit 3f08d92SummaryNOTE: some test results have been updated!
Detailed failure logs
Job: Set notifications [Build seL4test] This is the most I can report on right now, sorry! |
Test results for commit 3f08d92SummaryNOTE: some test results have been updated!
Detailed failure logs
Job: gcc-rv64imac-hw-build This is the most I can report on right now, sorry! |
Test results for commit 3f08d92SummaryNOTE: some test results have been updated!
Detailed failure logs
Job: gcc-rv64imac-hw-build This is the most I can report on right now, sorry! |
|
All the tests we ran have passed! Nice job! |
1 similar comment
|
All the tests we ran have passed! Nice job! |
|
merged! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Lukas Graber lukas.graber@hensoldt-cyber.de