-
Notifications
You must be signed in to change notification settings - Fork 26
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.
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
Support for ARM Neoverse N1 platform #381
Conversation
fbd8529
to
4b59510
Compare
4b59510
to
fc32287
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For documentation, some updates are missing:
- Please update the supported architectures in the README in the "Platform and Microarchitecture Support" section.
- Please update the supported architectures sections for the APIs added in
variorum.h
, so doxygen picks it up in our documentation (see here: https://github.com/LLNL/variorum/blob/dev/src/variorum/variorum.h#L296)
src/variorum/ARM/config_arm.c
Outdated
uint64_t *model = (uint64_t *) malloc(sizeof(uint64_t)); | ||
*model = ARMV8; | ||
unsigned long *model = (unsigned long *) malloc(sizeof(uint64_t)); | ||
asm volatile( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this change, @amarathe84, I assume you have thoroughly tested the part where it picks up the model. Is there documentation on this that we can add somewhere?
Also, I noticed that for Juno r2, you now have a different model for the big and the little processors, as opposed to just one which we had before. Can you elaborate why? Should we be representing them as the same model as we have been viewing the big.Little as a single entity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the PR description with the integration/regression tests on nhvpc1 and Juno r2. Please take a look at the test outcomes to see if they look okay.
Let me also post the description for the updated model ID check here shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the existing ARM implementation we assumed a generic ARMV8 (constant) model. To distinguish between ARM implementations, we need to look at the 'Primary part number' [15:4] bit fields of the MIDR_EL1 (Main ID) register which is defined per ARM CPU implementation (as opposed to a combined SoC like Juno r2). Here are the links to the MIDR_EL1 register for the three ARM CPU architectures we support:
Based on the model ID we set up the lower-level interfaces.
Juno r2 is a big.LITTLE implementation with both Cortex A72 and Cortex A53 in a single SoC. There are systems (e.g. revisions of Raspberry Pi) with either one of these two but with the same interfaces to lower-level functionality so the same code should work with them, but we haven't tested on such systems.
There's also a filesystem interface for MIDR_EL1 but I couldn't confirm if that's always available on an Arm implementation, so I went with the MRS instruction to get the model ID instead.
I'll add a subsection in the ARM Overview about model identification along with the links to ARM documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for explaining @amarathe84 ! This is very helpful for me to understand the detail of the model/arch_id. And yes, this would be great to document somewhere outside of this issue too in the ARM documentation in some way. I didn't know about the MIDR_EL1 register.
ARM_CORTEX_A72 = 0xd08, //ARM Cortex-A72 MPCore processor | ||
ARM_CORTEX_A53 = 0xd03, //ARM Cortex-A53 MPCore processor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these separate now, shouldn't we be representing the big.Little Juno r2 device as a single entity (we did this in the past)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: The model check in config_arm.c
is for either 0xd08 or 0xd03 is because the mrs %0, MIDR_EL1 =r<model>
may return either of the values depending on which CPU runs it at runtime by the OS scheduler. So as long as we pick up one of these model IDs, we know that it's not Neoverse N1 and proceed with using the sysfs interface.
Specifically for Juno r2 we could check for both A53 and A72 CPUs in the big.LITTLE SoC, and not for either one of the ARM CPUs but the change may be non-trivial since detect_arm_arch()
needs topological information (i.e. a list of CPUs present on the system) to run on both CPUs sequentially. Should I explore that or does the existing check suffice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this would address my concern about checking it as a single entity. Thanks @amarathe84 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the detect_arm_arch()
function to check the CPU ID of big and LITTLE clusters using the sysfs
interface for midr_el
register. The fix works for both Neoverse N1 and Juno r2. I did notice that the file I/O has slowed down the architecture check but that's the only way to simplify the logic. All tests worked as expected.
@amarathe84 Can you look at the comments here, and then we can merge? |
Updated both README.md and |
Looks good to me @amarathe84! I am curious about the model description check, but the PR is good to go I think. |
Description
This WIP PR introduces code contributions to support the Ampere Neoverse N1 ARM SoC platform. The code changes will introduce ARM CPU version check in order to bind the appropriate low-level functionality to the higher-level API. This WIP PR will implement the low-level functions in Variorum to expose the power features supported by the Neoverse N1 platform. Code refactoring will be done to split the code base for ARM based on the specific ARM platform.
Fixes #378, #379
Task checklist
Testing
Unit and component testing will be done using Variorum example programs on the following systems
Nvhpc1 system integration tests (Ampere Neoverse N1)
Print power usage
Print CPU temperature
Print CPU frequency
Cap CPU frequency
Juno system integration and regression tests (Arm Juno r2)
Print power usage
Print CPU temperature
Print CPU frequency
Cap CPU frequency