Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RFC] test: Add arm64_high_addresses test #33631

Merged
merged 1 commit into from
Aug 30, 2021
Merged

[RFC] test: Add arm64_high_addresses test #33631

merged 1 commit into from
Aug 30, 2021

Conversation

carlocaione
Copy link
Collaborator

First of all I'm not exactly sure if this is the best way to tackle this problem but at least it's something. @nashif (or any other CI expert) please let me know if you think there is a better way to do this.

So, the problem is that in the past couple of months we hit several time several issues related to having the SRAM to high addresses in memory or when SRAM_BASE_ADDRESS != KERNEL_VM_BASE: see #33607, #33594, #33204, #32053.

We do not have a practical way to actual test zephyr in emulation when the SRAM is mapped to high addresses but at least we can try to catch early problems at compile time that would have spared a bit of head-scratch in the past.

So this test is basically the build-only copy of the hello_world_user test but using high addresses for SRAM_BASE_ADDRESS and KERNEL_VM_BASE in several combinations.

@github-actions github-actions bot added the area: Tests Issues related to a particular existing or missing test label Mar 23, 2021
@carlocaione
Copy link
Collaborator Author

Of course this is failing on CI because #33594 has not been merged yet (and this proves that this works ;)

@npitre
Copy link
Collaborator

npitre commented Mar 23, 2021 via email

@carlocaione
Copy link
Collaborator Author

I wonder if arch_allow: arm won't let ARM32 through where this test is definitely wrong.

The filter: CONFIG_64BIT should be enough to avoid building for ARM32

And if we're going to have "low" addresses, I'd use something actually low i.g. 0x400000 instead of 0x40000000.

Yup, makes sense.

@carlocaione carlocaione mentioned this pull request Mar 23, 2021
Closed
@zephyrbot zephyrbot added the area: ARM64 ARM (64-bit) Architecture label Mar 23, 2021
@carlocaione carlocaione force-pushed the arm64_high_addresses branch from 3c6df2a to 4574afd Compare March 23, 2021 20:29
@carlocaione carlocaione force-pushed the arm64_high_addresses branch from 4574afd to cd3565e Compare April 1, 2021 08:45
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jul 14, 2021
@cfriedt
Copy link
Member

cfriedt commented Jul 25, 2021

@carlocaione - can you rebase? I see nothing wrong with additional tests.

@github-actions github-actions bot removed the Stale label Jul 26, 2021
@carlocaione carlocaione force-pushed the arm64_high_addresses branch from cd3565e to d50862e Compare July 26, 2021 07:24
@carlocaione
Copy link
Collaborator Author

@cfriedt can we merge this?

stephanosio
stephanosio reviewed Aug 24, 2021
View reviewed changes
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok overall. Some comments about the CMakeLists.

tests/arch/arm64/arm64_high_addresses/CMakeLists.txt Outdated Show resolved Hide resolved
tests/arch/arm64/arm64_high_addresses/CMakeLists.txt Outdated Show resolved Hide resolved
tests/arch/arm64/arm64_high_addresses/CMakeLists.txt Outdated Show resolved Hide resolved
@carlocaione carlocaione force-pushed the arm64_high_addresses branch from d50862e to b8d4e59 Compare August 26, 2021 14:33
@stephanosio stephanosio requested a review from tejlmand August 30, 2021 11:32
@carlocaione
test: Add arm64_high_addresses test
ad85dca 
Latel we have had several failures and regressions due to the setting of
CONFIG_SRAM_BASE_ADDRESS to really high values (over the 4GB boundary).
To try to catch these problems as early as possible we add a build-only
test based on the hello_world_user sample that tries to compile the test
using a combination of CONFIG_SRAM_BASE_ADDRESS and
CONFIG_KERNEL_VM_BASE set to high values in memory.

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
@carlocaione carlocaione force-pushed the arm64_high_addresses branch from b8d4e59 to ad85dca Compare August 30, 2021 11:56
@nashif nashif merged commit 0f3d2d9 into zephyrproject-rtos:main Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephanosio stephanosio stephanosio left review comments

@dcpleung dcpleung dcpleung approved these changes

@npitre npitre npitre approved these changes

@nashif nashif Awaiting requested review from nashif

@ioannisg ioannisg Awaiting requested review from ioannisg

@galak galak Awaiting requested review from galak

@tejlmand tejlmand Awaiting requested review from tejlmand

Assignees

@carlocaione carlocaione

Labels
area: ARM64 ARM (64-bit) Architecture area: Tests Issues related to a particular existing or missing test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@carlocaione @npitre @cfriedt @stephanosio @dcpleung @nashif @zephyrbot