Skip to content

ArmPkg/Drivers/ArmGicDxe: Add Extended SPI support for GICv3 #11032

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
Merged
merged 2 commits into from
Jun 3, 2025
Merged

ArmPkg/Drivers/ArmGicDxe: Add Extended SPI support for GICv3 #11032

merged 2 commits into from
Jun 3, 2025

Conversation

NicholasGraves
Copy link
Contributor

@NicholasGraves NicholasGraves commented May 2, 2025 •
edited
Loading

  • Add ESPI configuration to GICv3.
  • Remove mNumGicInterrupts and replace with GicIsValidSource(), GicGetGreatestIntid() APIs. mNumGicInterrupts is still used internally in GICv2.
  • Tested on qemu with the BSA test suite.

Description

Adds ESPI support to GICv3
#10869

  • 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

Tested on qemu with ESPI support via the BSA ACS test suite.

Integration Instructions

N/A

@NicholasGraves NicholasGraves changed the title #10869 ArmPkg/Drivers/ArmGicDxe: Add ESPI support for GICv3 ArmPkg/Drivers/ArmGicDxe: Add ESPI support for GICv3 May 5, 2025
@NicholasGraves NicholasGraves force-pushed the espi-upstream branch 2 times, most recently from a450368 to a916600 Compare May 7, 2025 00:35
@leiflindholm
Copy link
Member

Thanks for this, it's definitely a gap that needs closing.

I'll do a more detailed review later today, but two high-level comments:

  • ESPI is a heavily clashing acronym. This is probably why the GIC ARM consistently refers to it as Extended SPI. I think that would be helpful to do here too.
  • Is there any way to break this up into a few more commits? Bullet points in commit messages frequently indicate there is.

@NicholasGraves
Copy link
Contributor Author

Thanks for this, it's definitely a gap that needs closing.

I'll do a more detailed review later today, but two high-level comments:

  • ESPI is a heavily clashing acronym. This is probably why the GIC ARM consistently refers to it as Extended SPI. I think that would be helpful to do here too.
  • Is there any way to break this up into a few more commits? Bullet points in commit messages frequently indicate there is.

Changed ESPI -> ExtSpi in code and extended SPI in comments.

I don't think it's possible to easily break this up into atomic changes that are all functional.

@NicholasGraves NicholasGraves changed the title ArmPkg/Drivers/ArmGicDxe: Add ESPI support for GICv3 ArmPkg/Drivers/ArmGicDxe: Add Extended SPI support for GICv3 May 19, 2025
@ardbiesheuvel
Copy link
Member

@samimujawar @pierregondois @LeviYeoReum any comments on this series?

samimujawar
samimujawar reviewed May 20, 2025
View reviewed changes
Copy link
Contributor

@samimujawar samimujawar left a comment

Choose a reason for hiding this comment

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

Thank you for this patch.
Some minor suggestions otherwise this patch looks good to me.

@NicholasGraves NicholasGraves force-pushed the espi-upstream branch 2 times, most recently from 3b7301e to a046766 Compare May 21, 2025 23:29
samimujawar
samimujawar reviewed May 22, 2025
View reviewed changes
@NicholasGraves
Copy link
Contributor Author

@samimujawar any more issues to address?

samimujawar
samimujawar reviewed May 30, 2025
View reviewed changes
Copy link
Contributor

@samimujawar samimujawar left a comment

Choose a reason for hiding this comment

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

Other than the initialisation of mGicOperationConfigMap[], this patch looks good to me.

@NicholasGraves NicholasGraves force-pushed the espi-upstream branch 5 times, most recently from c184d06 to 4926f8b Compare June 2, 2025 16:41
@ardbiesheuvel ardbiesheuvel added the push Auto push patch series in PR if all checks pass label Jun 2, 2025
@NicholasGraves
ArmPkg/Drivers/ArmGicDxe: Add Extended SPI support for GICv3
f372752 
This commit enables extended SPI support for GicV3. GicV3 must decide,
based on the source intid, whether to access the SPI-range registers,
PPI-range registers in the redistributor, or the extended SPI-range
registers.

The protocol interfaces must also support registering an interrupt
handler with an extended SPI intid. To save ~24KB of memory, handler
allocation and access is delegated to GicV2 and GicV3. GicV2 retains the
existing handler mapping scheme using intids literally. GicV3 remaps
extended SPI intids to be immediately after the highest SPI intids.

Tested on qemu with the BSA test suite.

Signed-off-by: Nick Graves <[email protected]>
@mergify mergify bot merged commit d6d2f68 into tianocore:master Jun 3, 2025
126 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samimujawar samimujawar samimujawar left review comments

@ardbiesheuvel ardbiesheuvel ardbiesheuvel approved these changes

@leiflindholm leiflindholm Awaiting requested review from leiflindholm

Assignees
No one assigned
Labels
push Auto push patch series in PR if all checks pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NicholasGraves @leiflindholm @ardbiesheuvel @samimujawar