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.

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

Fix ARM CPUID printing #107

Merged
merged 1 commit into from
Sep 2, 2021
Merged

Conversation

bennoleslie
Copy link

The existing print_cpuid function displays the incorrect value
for Cortex-A35. (See issue #43).

This change makes CPUID determination an explicit switch statement
rather than attempting to automatically determine it through
masking certain bits.

Note: At some level this will be a regression as it now only
prints the textual id for Cortex-A35 and no other CPU.

Improvements to list other CPUID mappings is welcome, unfortunately
the only platform I have access to for testing is Cortex-A35.

@lsf37 lsf37 requested a review from axel-h August 30, 2021 23:05
@lsf37 lsf37 self-assigned this Aug 30, 2021
@axel-h
Copy link
Member

axel-h commented Aug 31, 2021

This patch fixes A35, but breaks a lot of the rest. There is a list in last column of the table at http://en.wikipedia.org/wiki/Comparison_of_ARMv8-A_cores that needs to be worked in here.

@bennoleslie
Copy link
Author

This patch fixes A35, but breaks a lot of the rest. There is a list in last column of the table at http://en.wikipedia.org/wiki/Comparison_of_ARMv8-A_cores that needs to be worked in here.

Added, but obviously untested. I think they are reasonably clear so can be merged without testing all cases.

@axel-h
Copy link
Member

axel-h commented Sep 2, 2021

Thanks, to merge there are two things that should be addressed:

  • All these ID are only valid if CPUID_IMPL(cpuid) == CPUID_IMPL_ARM. Otherwise we should just print the raw value.
  • This still lacks all the ARMv7 cores
    • 0xc05: "Cortex-A5"
    • 0xc07: "Cortex-A7"
    • 0xc08: "Cortex-A8"
    • 0xc09: "Cortex-A9"
    • 0xc0d: "Cortex-A12"
    • 0xc0f: "Cortex-A15"
    • 0xc0e: "Cortex-A17"

These things are nice to have:

  • There are also a few ARMv8 cores out there that we may want to add:
    • 0xd42: "Cortex-A78AE"
    • 0xd4b: "Cortex-A78C"
  • This seems to be the list for the brand new ones:
    • 0xd0c, "Neoverse N1"
    • 0xd40: "Neoverse V1"
    • 0xd46: "Cortex-A510"
    • 0xd47: "Cortex-A710"
    • 0xd48: "Cortex-X2"
    • 0xd49: "Neoverse N2"
    • 0xd4a: "Neoverse E1"

bennoleslie pushed a commit to BreakawayConsulting/seL4_tools that referenced this pull request Sep 2, 2021
@bennoleslie
Copy link
Author

Thanks, to merge there are two things that should be addressed:

  • All these ID are only valid if CPUID_IMPL(cpuid) == CPUID_IMPL_ARM. Otherwise we should just print the raw value.

I don't think this needs to be addressed to merge. This is a bug (that was preexisting). I think we should create a bug in the issue tracker and address this separately.

Doing that properly would be a refactor of the part lookup into a cpuid_get_arm_part_str(...) function. Which is a good idea. I'd just like to keep the PRs separate.

Copy link
Member

@axel-h axel-h left a comment

Choose a reason for hiding this comment

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

Look good please add the missing DCO and consider squashing the commits.

The existing print_cpuid function displays the incorrect value
for Cortex-A35. (See issue seL4#43).

This change makes CPUID determination an explicit switch statement
rather than attempting to automatically determine it through
masking certain bits.

Note: Only the Cortex-A35 case in the switch statement has
been tested. Others cases are implemented from available
lists only and have not been validated on real hardware.

Sources:

https://en.wikipedia.org/wiki/Comparison_of_ARMv8-A_cores
seL4#107 (comment)

Signed-off-by: Ben Leslie <benno@brkawy.com>
@bennoleslie
Copy link
Author

Look good please add the missing DCO and consider squashing the commits.

Done.

@lsf37 lsf37 merged commit c5f0e54 into seL4:master Sep 2, 2021
@bennoleslie bennoleslie deleted the fix-cpu-id-print branch September 2, 2021 07:28
sleffler pushed a commit to AmbiML/sparrow-seL4_tools that referenced this pull request Oct 11, 2022
The existing print_cpuid function displays the incorrect value
for Cortex-A35. (See issue seL4#43).

This change makes CPUID determination an explicit switch statement
rather than attempting to automatically determine it through
masking certain bits.

Note: Only the Cortex-A35 case in the switch statement has
been tested. Others cases are implemented from available
lists only and have not been validated on real hardware.

Sources:

https://en.wikipedia.org/wiki/Comparison_of_ARMv8-A_cores
seL4#107 (comment)

Signed-off-by: Ben Leslie <benno@brkawy.com>
GitOrigin-RevId: c5f0e54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants