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

ArmPkg: Update a few casting instances #6307

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

ArmPkg: Update a few casting instances #6307

wants to merge 7 commits into from
+27 −31

Conversation

kuqin12
Copy link
Contributor

@kuqin12 kuqin12 commented Oct 10, 2024 •
edited
Loading

Description

The existing ArmPkg implementation has some implicit casting and even truncation. Some caused build failures on non-GCC compilers. This change intends to fix those issues.

  • Breaking change?
    • Breaking change - Does this PR cause a break in build or boot behavior?
    • Examples: Does it add a new library class or move a module to a different repo.
  • Impacts security?
    • Security - Does this PR have a direct security impact?
    • Examples: Crypto algorithm change or buffer overflow fix.
  • Includes tests?
    • Tests - Does this PR include any explicit test code?
    • Examples: Unit tests or integration tests.

How This Was Tested

This change is tested on QEMU SBSA platform.

Integration Instructions

N/A

@github-actions github-actions bot added the impact:breaking-change This change breaks existing APIs impacting build or boot. label Oct 10, 2024
@leiflindholm
Copy link
Member

I'm not convinced the breaking-change label should be set. What does this set break?

@kuqin12
Copy link
Contributor Author

kuqin12 commented Oct 18, 2024

I'm not convinced the breaking-change label should be set. What does this set break?

I marked it as breaking change mainly because there is public interface update inside ArmLib. Do we not mark an implicit integer promotion change as breaking?

@leiflindholm
Copy link
Member

I'm not convinced the breaking-change label should be set. What does this set break?

I marked it as breaking change mainly because there is public interface update inside ArmLib. Do we not mark an implicit integer promotion change as breaking?

For ArmCacheWritebackGranule()?
I'd argue it's not breaking. I can't see any existing code that would need to be updated to work correctly after the type changes from UINTN to UINT32. However, looking at that patch it kind of jumps out at me that only the declaration in .h file is updated, without updating the implementation. That needs fixing.

@kuqin12
Copy link
Contributor Author

kuqin12 commented Nov 13, 2024 •
edited
Loading

I'm not convinced the breaking-change label should be set. What does this set break?

I marked it as breaking change mainly because there is public interface update inside ArmLib. Do we not mark an implicit integer promotion change as breaking?

For ArmCacheWritebackGranule()? I'd argue it's not breaking. I can't see any existing code that would need to be updated to work correctly after the type changes from UINTN to UINT32. However, looking at that patch it kind of jumps out at me that only the declaration in .h file is updated, without updating the implementation. That needs fixing.

Agreed. The actual value in the existing implementation will not exceed the UINT32 limit, which was why it was kept as it was. But I updated the implementation to match UINT32 return anyway.

Also, the breaking change label is removed.

@kuqin12 kuqin12 marked this pull request as ready for review November 13, 2024 18:12
@Javagedes Javagedes removed the impact:breaking-change This change breaks existing APIs impacting build or boot. label Nov 13, 2024
@kuqin12
ArmPkg: ArmGic: Cast CpuTarget to UINT32 for legacy GIC
b46c396 
The current code path supporting `PcdArmGicV3WithV2Legacy` will read 32
bit CPU target and try to program ARM_GIC_ICDIPTR. However, all these
operations are 32bit wide.

This change casts the CpuTarget variable to be UINT32 before calling
MMIO read.

Cc: Leif Lindholm <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Sami Mujawar <[email protected]>

Signed-off-by: Kun Qin <[email protected]>
@kuqin12
ArmPkg: TimerDxe: Use 64bit operation for timer ticks
01bf85b 
The current implementation operates on 64bit value with implicit value
truncation.

This change updates the involved frequencies to use 64 bit based
operations.

Cc: Leif Lindholm <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Sami Mujawar <[email protected]>

Signed-off-by: Kun Qin <[email protected]>
@kuqin12
ArmPkg: ArmArchTimerLib: Update operations to be 64 bit wide
1fc2d14 
The existing operation in ArmArchTimerLib is operating on UINT32 or
UINT64 based on the target system. This casting game originates from the
fact that timer frequency is UINTN type.

This change will simply promote all operations to UINT64 based, which
will remove the casting and conditional #if in the code for better
portability and readability.

Cc: Leif Lindholm <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Sami Mujawar <[email protected]>

Signed-off-by: Kun Qin <[email protected]>
@kuqin12
MdePkg: ArmLib: Return UINT32 for ArmCacheWritebackGranule
0fff613 
ArmCacheWritebackGranule should not return value higher than MAX_UINT32.

This change will allow the usage without architecture depenedent return
size.

Cc: Leif Lindholm <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Sami Mujawar <[email protected]>

Signed-off-by: Kun Qin <[email protected]>
@kuqin12
ArmPkg: ArmLib: Update function to match header file
19e3e2f 
Update function implementation to match interface definition. The return
should be bound to 0xffff0000, which is guaranteed to be a UINT32.

Cc: Leif Lindholm <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Sami Mujawar <[email protected]>

Signed-off-by: Kun Qin <[email protected]>
@kuqin12
ArmPkg: ArmExceptionLib: Fixing exception vector and type casting
3ecd249 
The current VectorBase is taking value from a 64bit PCD into a UINTN
value, which could have truncated value for 32bit system.

In addition, the comparison between UINTN and INTN could lead to
undesired comparison outcome or compiler complaints.

This change updates all of them to be UINT64 based operation.

Cc: Leif Lindholm <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Sami Mujawar <[email protected]>

Signed-off-by: Kun Qin <[email protected]>
@kuqin12
ArmPkg: DefaultExceptionHandlerLib: Update function input bitwidth
f6ec2b9 
The current DescribeExceptionSyndrome is taking ESR as 32bit value.
However, ESR should be a 64 bit value. This change updates the function
to intake a 64bit value.

Cc: Leif Lindholm <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Sami Mujawar <[email protected]>

Signed-off-by: Kun Qin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leiflindholm leiflindholm leiflindholm left review comments

@ardbiesheuvel ardbiesheuvel Awaiting requested review from ardbiesheuvel

@lgao4 lgao4 Awaiting requested review from lgao4

@LiuZhiguang001 LiuZhiguang001 Awaiting requested review from LiuZhiguang001

@mdkinney mdkinney Awaiting requested review from mdkinney

@samimujawar samimujawar Awaiting requested review from samimujawar

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kuqin12 @leiflindholm @Javagedes