Skip to content

cca-irq: add initial GICv3 delivery for timer, SGI, and SPI - #4289

Open
Wei Ding (weiding-msft) wants to merge 1 commit into
microsoft:mainfrom
weiding-msft:irq-dev-merging
Open

cca-irq: add initial GICv3 delivery for timer, SGI, and SPI#4289
Wei Ding (weiding-msft) wants to merge 1 commit into
microsoft:mainfrom
weiding-msft:irq-dev-merging

Conversation

@weiding-msft

@weiding-msft Wei Ding (weiding-msft) commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Add the initial ARM GICv3 interrupt-delivery framework for CCA lower planes. Route virtual timer PPIs, self-generated SGIs, and shared SPIs through a partition-owned GIC model and inject eligible interrupts through the RMM GIC list registers.

Previously, CCA IRQ exits were only logged, trapped GIC accesses did not provide a complete interrupt path, and local interrupt sources could be inserted into an LR without respecting the guest-programmed GIC enable and priority state.

CCA interrupt injection

Add a common CCA virtual-interrupt representation containing an INTID, priority, group, and pending state.

Preserve the GIC list registers returned by the RMM and inject pending software-backed Group 1 interrupts into the first free LR. Avoid adding a duplicate LR when the same INTID is already pending or active.

Enable ICH_HCR_EL2.TC for plane entry and encode software LRs with:

  • the virtual INTID in bits 31:0
  • the guest-programmed priority in bits 55:48
  • Group 1 in bit 60
  • HW clear in bit 61
  • Pending state in bits 63:62

Leave EOI-maintenance disabled because the CCA backend does not yet implement the GIC maintenance-interrupt path.

Use an invalid exit-reason sentinel to distinguish a real plane exit from a host wakeup, and preserve the RMM-returned plane and LR context before processing the exit.

Virtual timer PPI

Handle PlaneExitReason::Irq and recognize an asserted architectural virtual timer from the RMM-provided CNTV_CTL_EL0 state.

Request the timer PPI only when:

  • ENABLE is set
  • IMASK is clear
  • ISTATUS is set

Obtain the virtual timer INTID from ProcessorTopology instead of hardcoding PPI 20. This is required because the RMM timer exit reports the timer source but does not provide an INTID.

Queue the timer as a private interrupt and pass it through the shared GIC redistributor selection path before LR injection. Delivery therefore now observes:

  • distributor Group 1 enable
  • GICR_IGROUPR0
  • GICR_ISENABLER0
  • GICR active state
  • the PPI priority programmed through GICR_IPRIORITYR
  • the priority threshold programmed through ICC_PMR_EL1

Decode trapped ICC_PMR_EL1 writes and retain the current priority mask for each CCA VTL. This fixes timer PPIs bypassing guest GIC enable and priority configuration.

SGI

Decode synchronous ICC_SGI1R_EL1 system-register traps and queue the requested self-SGI rather than injecting it directly.

Advance the lower-plane PC after emulating ICC_SGI1R_EL1 and ICC_PMR_EL1 writes so that the trapped instruction is not executed again after re-entry.

Route queued SGIs through the same private-interrupt selection used by PPIs. SGI delivery therefore also observes the redistributor enable, Group 1, active, programmed priority, and ICC_PMR_EL1 state.

Add a TMK self-SGI test using INTID 5 and the Group 1 system-register interface.

SPI

Create a partition-owned GicV3Model for CCA and expose it to the VP runner so that device interrupt assertion, CCA interrupt polling, and guest GIC MMIO emulation operate on the same state.

Implement the CCA ControlGic::set_spi_irq path. Record the device line level, determine the target VPs from GICD_IROUTER state, and wake those VPs through the interrupt-controller wake reason when a line is newly asserted.

Track SPI state independently as:

  • software pending state
  • external line assertion level
  • active state
  • LR/in-flight ownership

Select a shared interrupt only when it is pending or asserted, enabled, assigned to Group 1, inactive, routed to the current VP, not already in flight, and has a priority that passes ICC_PMR_EL1.

Preserve an edge-triggered SPI request after the source line falls by latching it in the pending state until successful LR injection.

Preserve the asserted state of a level-triggered SPI after injection. When the RMM retires its LR, clear only the in-flight ownership. If the device line remains asserted, make the SPI eligible for injection again. Stop redelivery only after the device deasserts the line.

This fixes level-triggered SPIs being lost after their first injection and EOI.

Support guest software pending and clearing through GICD_ISPENDR and GICD_ICPENDR. Deasserting a device line does not clear an independently created software-pending request.

GIC model and TMK support

Extend the reusable GICv3 model with the distributor and redistributor state needed by the CCA paths, including:

  • Group 1 distributor enable
  • interrupt group and enable state
  • pending, active, and trigger configuration
  • per-INTID priorities
  • SPI routing
  • GICR SGI/PPI enable, group, pending, active, and priority state
  • priority-based private and shared interrupt selection
  • MMIO reads and writes for the supported GICD/GICR registers

Define a common TMK AArch64 platform layout:

  • GICD at 0xff00_0000
  • GICR at 0xff02_0000
  • virtual timer PPI 20
  • 256 exposed INTIDs: SGI 0-15, PPI 16-31, and SPI 32-255

Add minimal EL1 IRQ support to tmk_core:

  • install an aligned VBAR_EL1 exception-vector table
  • save and restore registers around IRQ handling
  • provide scoped IRQ callback registration
  • configure GIC Group 1, interrupt enable, and priority state
  • program ICC_SRE_EL1, ICC_PMR_EL1, and ICC_IGRPEN1_EL1
  • acknowledge through ICC_IAR1_EL1
  • complete through ICC_EOIR1_EL1
  • mask and unmask IRQs through DAIF
  • provide virtual timer, self-SGI, and software-pending SPI helpers

Route TMK GIC MMIO accesses to the same partition GicV3Model used by the CCA VP. Add tests for:

  • architectural virtual timer PPI delivery
  • self-targeted SGI delivery
  • software-pended SPI 32 delivery
  • level-triggered SPI redelivery until deassertion
  • edge-triggered SPI pending latching
  • preservation of software-pending state across line deassertion
  • SGI/PPI enable and priority filtering

Correct the RSI plane-exit layout so the timer state starts at its ABI-defined 0x400 offset and assert the resulting structure offsets and sizes.

Known limitations

This is an initial CCA IRQ implementation, not a complete GICv3 implementation.

  • PlaneExitReason::Irq currently recognizes only the architectural virtual timer. PMU, physical timer, and other local PPI sources are not identified or mapped.
  • SGI support is limited to the single-VP/self-target test path. Full ICC_SGI1R_EL1 affinity routing, target lists, broadcast behavior, and multi-VP delivery are not implemented or validated.
  • The SPI end-to-end test uses GICD_ISPENDR to create a software-pending SPI. It does not validate a real emulated device asserting and deasserting ControlGic::set_spi_irq through the complete device/VMM/ Realm path.
  • Level- and edge-triggered device SPI behavior is covered by GIC model unit tests, but still needs end-to-end device validation, including routing, VP wakeup, EOI/LR retirement, and repeated level delivery.
  • The TMK platform currently creates one VP and exposes only INTIDs 0-255. INTIDs 256-1019 are not implemented by this test platform.
  • Only the GIC register subset required by the current tests is implemented. Group 0, complete security-state behavior, ITS/MSI/LPI, and full architectural GICv3 semantics are not supported.
  • LR allocation is first-free. There is no LR eviction, maintenance interrupt handling, or complete priority-preemption model.
  • Private pending storage is bounded, and running-priority, binary-point, and nested-interrupt behavior are not fully modeled.
  • GIC EOI-maintenance events, cross-VTL interrupts, and the CCA untrusted-SynIC interrupt path remain unimplemented.
  • MMIO polling in the TMK tests is a workaround for current RMM/FVP notification behavior rather than the final asynchronous delivery mechanism.
  • Unsupported CCA IRQ sources are traced and ignored, while unsupported system-register traps terminate the VP.

IRQ tests

Add TMK tests covering the initial CCA interrupt-delivery paths:

  • Virtual timer PPI: program CNTV_CVAL_EL0, wait for virtual timer PPI 20,
    verify delivery through the GIC, and disable the timer in the IRQ handler.

  • Self-targeted SGI: generate Group 1 SGI 5 through ICC_SGI1R_EL1 and verify
    that it is delivered to the current VP.

  • Software-pended SPI: set SPI 32 pending through GICD_ISPENDR and verify
    delivery through the distributor and GIC list-register path.

    Install the CCA emulation environment once with:

      cargo xflowey cca-tests --install-emu
    

    Build and run the CCA runtime test, including these TMK IRQ tests, with:

      cargo xflowey cca-tests
    

    To build the test artifacts without running the emulator, use:

      cargo xflowey cca-tests --build-only
    

    Note taht "Software-pended SPI” is intentionally precise: this test does not
    exercise a real emulated device asserting an SPI line.

Test results

INFO test: test passed, name: "aarch64::irq::virtual_timer_irq"
INFO test: test passed, name: "aarch64::irq::spi_self_pending_irq"
INFO test: test passed, name: "aarch64::irq::sgi_self_irq"

Signed-off-by: Wei Ding b-weiding@microsoft.com
Signed-off-by: Ben Aram b-bearam@microsoft.com
Signed-off-by: Jiong Wang b-jiongwang@microsoft.com

Copilot AI lite review requested due to automatic review settings August 20, 2026 04:48
@github-actions github-actions Bot added the unsafe Related to unsafe code label Aug 20, 2026
@github-actions

Copy link
Copy Markdown

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds an initial ARM GICv3 interrupt delivery path for CCA lower planes, including software-backed interrupt selection/injection (via LRs) and a shared, partition-owned GIC model used consistently across emulation and interrupt assertion.

Changes:

  • Introduces a reusable GicV3Model with pending selection for private interrupts and SPIs, plus SPI line-level/in-flight tracking in the distributor.
  • Extends the CCA backend to queue/inject timer PPIs and self-SGIs, preserve LR context across exits, and integrate SPI assertion/wakeup via ControlGic::set_spi_irq.
  • Adds TMK AArch64 platform constants and minimal EL1 IRQ plumbing + tests for timer/SGI/SPI delivery.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
vmm_core/virt_support_gic/src/lib.rs Adds GICv3 model wrapper, SPI assertion/in-flight tracking, and pending selection APIs.
vm/aarch64/aarch64defs/src/rsi.rs Fixes RSI plane-exit layout/offset assertions for timer state ABI.
tmk/tmk_vmm/src/run.rs Hooks TMK VMM MMIO to the shared GIC model and uses shared platform constants.
tmk/tmk_vmm/src/paravisor_vmm.rs Propagates the partition-owned GIC model into TMK runners (AArch64).
tmk/tmk_vmm/Cargo.toml Adds deps required for AArch64 GIC integration in TMK VMM.
tmk/tmk_protocol/src/lib.rs Defines fixed AArch64 TMK platform layout constants (GIC bases/INTIDs).
tmk/tmk_core/src/lib.rs Exposes AArch64 module for use by TMKs (behind target_arch).
tmk/tmk_core/src/aarch64.rs Implements minimal EL1 IRQ vectoring, handler registration, and GIC helpers.
tmk/simple_tmk/src/aarch64/mod.rs Wires in new AArch64 IRQ tests module.
tmk/simple_tmk/src/aarch64/irq.rs Adds TMK tests for virtual timer PPI, self SGI, and software-pended SPI.
openhcl/virt_mshv_vtl/src/processor/cca/mod.rs Implements CCA interrupt queuing/injection, sysreg trap decode, and GIC LR preservation.
openhcl/virt_mshv_vtl/src/lib.rs Adds partition-owned GIC model for CCA and routes set_spi_irq through it.
openhcl/virt_mshv_vtl/Cargo.toml Adds virt_support_gic dependency for AArch64/CCA builds.
Cargo.lock Records new dependency edges (virt_support_gic, aarch64defs) for affected crates.
Suppressed comments (1)

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:772

  • ICC_SGI1R_EL1 decoding treats any non-zero target_list (and IRM/broadcast) as a self-SGI. This will incorrectly inject SGIs for unsupported routing modes / multi-target lists into the current VP. If only self-target is supported right now, validate and reject unsupported encodings (or at least ignore empty target lists with an accurate message).
        if sgi.irm() || sgi.target_list() != 0 {
            let vtl = self.backing.cvm.exit_vtl;
            self.request_gic_interrupt(vtl, intid, dev)?;
            tracing::debug!(
                intid,

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +733 to +735
return Err(
dev.fatal_error(CcaUnsupportedExit::InvalidSgiValue(exit_esr_el2).into())
);
Comment on lines +358 to +368
let intid = word as u32 * 32 + bit;
let mask = 1 << bit;
let pending = state.pending[word] & mask != 0;
let level_asserted =
state.asserted[word] & mask != 0 && !Self::edge_triggered(&state, intid);
if !pending && !level_asserted {
continue;
}
if intid > self.max_spi_intid
|| state.in_flight[intid as usize].is_some()
|| !self.spi_targets_vp(&state, intid, vp)
Copilot AI review requested due to automatic review settings August 20, 2026 18:43
@weiding-msft
Wei Ding (weiding-msft) force-pushed the irq-dev-merging branch 2 times, most recently from ff20de9 to bee9564 Compare August 20, 2026 18:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (3)

vmm_core/virt_support_gic/src/lib.rs:211

  • route is sized as n * 64, which allocates 2× more entries than the number of INTIDs (and makes it harder to reason about indexing, since spi_targets_vp indexes route by INTID). This should be sized to interrupt_count (one u64 per INTID).
                    cfg: vec![0; n * 2],
                    priority: vec![0; n * 8],
                    route: vec![0; n * 64],
                    enable_grp0: false,

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:736

  • When the trapped ICC_SGI1R_EL1 source register isn't available, this path returns InvalidSgiValue(exit_esr_el2), which both reports the wrong value (ESR instead of the SGI value) and uses a misleading error variant. This should use MissingSystemRegisterValue { system_reg, esr_el2: exit_esr_el2 }, consistent with the ICC_PMR_EL1 handling above.
                let Some(value) = source_value else {
                    tracing::warn!(
                        rt = iss.rt(),
                        "CCA ICC_SGI1R_EL1 write has source register outside RSI GPR array"
                    );
                    return Err(
                        dev.fatal_error(CcaUnsupportedExit::InvalidSgiValue(exit_esr_el2).into())
                    );

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:171

  • pending_mask builds a bitmask with 1 << intid, but request_interrupt doesn't enforce intid < 32. If a misconfigured timer PPI (or any future caller) passes an INTID >= 32, this shift can panic. Reject non-private INTIDs up front and make pending_mask use a checked shift to avoid panics.
    fn request_interrupt(&mut self, intid: u32) -> bool {
        if self.pending.contains(&Some(intid)) {
            return true;
        }

        let Some(slot) = self.pending.iter_mut().find(|slot| slot.is_none()) else {
            return false;
        };
        *slot = Some(intid);
        true
    }

    fn pending_mask(&self) -> u32 {
        self.pending
            .iter()
            .flatten()
            .fold(0, |pending, intid| pending | 1 << intid)
    }

Copilot AI review requested due to automatic review settings August 20, 2026 18:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (2)

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:735

  • On the ICC_SGI1R_EL1 write path, a missing source register currently returns InvalidSgiValue(exit_esr_el2), which both reports the wrong value (ESR instead of SGI value) and uses an error variant that doesn't match the failure. This should use the existing MissingSystemRegisterValue { system_reg, esr_el2 } error instead.
                    return Err(
                        dev.fatal_error(CcaUnsupportedExit::InvalidSgiValue(exit_esr_el2).into())
                    );

tmk/tmk_core/src/aarch64.rs:182

  • arch_init() records interrupts_enabled after arch_init_once() has potentially masked IRQs, so the saved state may always be "disabled" for the first scope. This contradicts the doc comment and prevents arch_reset() from restoring the pre-scope IRQ mask state correctly.
    pub(super) fn arch_init() -> ArchScopeState {
        arch_init_once();
        ArchScopeState {
            old_irq_handler: None,
            interrupts_enabled: are_interrupts_enabled(),

Copilot AI review requested due to automatic review settings August 20, 2026 19:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (3)

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:783

  • handle_icc_sgi1r_el1_write currently treats any non-empty target_list as targeting the current VP (sgi.target_list() != 0). With a single VP this can still incorrectly inject SGIs that target a non-existent affinity bit (e.g. target_list = 0b10), and with multiple VPs it would misdeliver SGIs broadly. If this path is intentionally limited to self-targeted SGIs, validate that the affinity fields match and that the current VP’s Aff0 bit is set (or ignore non-self targets).
        if sgi.irm() || sgi.target_list() != 0 {

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:740

  • When ICC_SGI1R_EL1 write traps but the source register is outside the RSI GPR array, this returns InvalidSgiValue(exit_esr_el2). That both reports the wrong datum (ESR instead of the SGI value) and uses an error variant intended for malformed SGI payloads rather than a missing source register. Consider reporting MissingSystemRegisterValue (like the ICC_PMR_EL1 path) so the failure mode is accurate and debuggable.
            SystemReg::ICC_SGI1R_EL1 if !iss.direction() => {
                let Some(value) = source_value else {
                    tracing::warn!(
                        rt = iss.rt(),
                        "CCA ICC_SGI1R_EL1 write has source register outside RSI GPR array"
                    );
                    return Err(
                        dev.fatal_error(CcaUnsupportedExit::InvalidSgiValue(exit_esr_el2).into())
                    );

tmk/tmk_vmm/Cargo.toml:13

  • tmk_vmm/Cargo.toml adds a direct dependency on aarch64defs, but there doesn’t appear to be any usage of aarch64defs in tmk/tmk_vmm (no references in src/). If it’s not needed, drop the dependency to keep the manifest minimal (and to avoid it being auto-pruned by repo formatting tools).
aarch64defs.workspace = true

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (3)

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:735

  • This error path reports InvalidSgiValue(exit_esr_el2), which both misclassifies the failure (the value is missing because rt is out of range) and passes the ESR instead of the ICC_SGI1R_EL1 value. Use the existing MissingSystemRegisterValue { system_reg, esr_el2 } variant here so the fatal error is accurate.
                    return Err(
                        dev.fatal_error(CcaUnsupportedExit::InvalidSgiValue(exit_esr_el2).into())
                    );

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:800

  • Logging every requested private interrupt at info can be extremely noisy (e.g., frequent virtual timer interrupts) and is guest-influenceable. This should be rate-limited or moved to a lower verbosity level to avoid log spam in production.
        tracing::info!(intid, ?vtl, "requested CCA private GIC interrupt");

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:768

  • ICC_SGI1R_EL1 writes are treated as targeting the current VP whenever irm() is set or target_list() != 0, but that does not actually check whether this VP is included in the target list. On multi-VP setups this can spuriously queue SGIs that were meant for other targets, despite the log message claiming otherwise.
        if sgi.irm() || sgi.target_list() != 0 {

Copilot AI review requested due to automatic review settings August 20, 2026 19:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (5)

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:735

  • The ICC_SGI1R_EL1 missing-source-register path reports InvalidSgiValue(exit_esr_el2), which both (a) uses ESR as the "value" in the error and (b) hides the real problem (the trapped instruction’s source register isn’t available). This makes diagnosis misleading and inconsistent with ICC_PMR_EL1 handling.
                    return Err(
                        dev.fatal_error(CcaUnsupportedExit::InvalidSgiValue(exit_esr_el2).into())
                    );

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:770

  • handle_icc_sgi1r_el1_write() treats ICC_SGI1R_EL1.IRM==1 as a self-targeted SGI (the log message even says "self SGI"). IRM changes the routing mode and target_list semantics; for the current single-VP/self-target implementation, IRM should be rejected/ignored rather than queued as self-target.
        if sgi.irm() || sgi.target_list() != 0 {
            let vtl = self.backing.cvm.exit_vtl;
            self.request_gic_interrupt(vtl, intid, dev)?;

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:800

  • This log is on a guest-triggerable path (virtual timer and SGIs) and will likely be very high frequency; emitting it at INFO can flood logs and impact performance. Consider lowering to TRACE/DEBUG or rate-limiting.
        tracing::info!(intid, ?vtl, "requested CCA private GIC interrupt");

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:171

  • pending_mask() builds a u32 bitmask by shifting 1 by each pending INTID. If an INTID >= 32 is ever queued, this shift can overflow/behave unexpectedly; using checked_shl avoids that and makes the private-interrupt assumption explicit.

This issue also appears in the following locations of the same file:

  • line 768
  • line 800
    fn pending_mask(&self) -> u32 {
        self.pending
            .iter()
            .flatten()
            .fold(0, |pending, intid| pending | 1 << intid)

vmm_core/virt_support_gic/src/lib.rs:454

  • spi_targets_vp() relies on state.route[intid] being populated from guest writes to GICD_IROUTER. However the current GICD IROUTER MMIO handler indexes using (r.0 & 0x1fff)/8, which maps IROUTER0 (0x6000) to index 0 and prevents INTID 32+ routes from being recorded/read back. As a result, SPI routing/wakeup decisions here can silently fall back to the default route value.
            let route = state.route.get(intid as usize).copied().unwrap_or(0);
            if route & (1 << 31) != 0 {
                return true;
            }

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (4)

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:732

  • In the ICC_SGI1R_EL1 trap path, the missing-source-register case returns InvalidSgiValue(exit_esr_el2), which both reports the wrong datum (ESR instead of the SGI value) and uses an error variant that doesn’t match the failure (the value is unknown, not invalid). This should mirror the ICC_PMR_EL1 handling and return MissingSystemRegisterValue.
                let Some(value) = source_value else {
                    tracing::warn!(
                        rt = iss.rt(),
                        "CCA ICC_SGI1R_EL1 write has source register outside RSI GPR array"
                    );

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:772

  • handle_icc_sgi1r_el1_write() queues a “self SGI” for any target_list != 0, even if the SGI targets a different affinity / doesn’t include the current VP. On a single-VP platform this means a guest can request an SGI to a non-existent target and still get an interrupt delivered, which is architecturally incorrect and can break guest expectations.
        if sgi.irm() || sgi.target_list() != 0 {
            let vtl = self.backing.cvm.exit_vtl;
            self.request_gic_interrupt(vtl, intid, dev)?;
            tracing::debug!(
                intid,

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:164

  • CcaGic::pending_mask() builds a 32-bit bitmask, but request_interrupt() currently accepts any intid. If an INTID >= 32 is ever queued here, pending_mask() will shift past the width of u32, producing incorrect masks (and potentially panicking in some build configurations). Since this queue is used for private interrupts (SGI/PPI), reject out-of-range INTIDs at the insertion point.

This issue also appears in the following locations of the same file:

  • line 728
  • line 768
    fn request_interrupt(&mut self, intid: u32) -> bool {
        if self.pending.contains(&Some(intid)) {
            return true;
        }

tmk/tmk_core/src/aarch64.rs:182

  • arch_init() records interrupts_enabled after calling arch_init_once(), but arch_init_once() masks IRQs. On first use this causes the scope to always record “interrupts were disabled”, so arch_reset() may fail to restore the pre-scope IRQ state correctly. Record the IRQ state before masking/initialization.
    pub(super) fn arch_init() -> ArchScopeState {
        arch_init_once();
        ArchScopeState {
            old_irq_handler: None,
            interrupts_enabled: are_interrupts_enabled(),

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.

@@ -70,7 +211,7 @@ mod gicd {
enable_grp0: false,
return Err(dev.fatal_error(err.into()));
}

tracing::info!(intid, ?vtl, "requested CCA private GIC interrupt");

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (1)

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:835

  • request_gic_interrupt logs every private interrupt request at info level. Timer PPIs and SGIs can occur frequently and are guest-triggerable, so this can create significant log volume and overhead. Consider downgrading this to trace (or only logging on error) to avoid log flooding in normal operation.
        tracing::info!(intid, ?vtl, "requested CCA private GIC interrupt");

@Brian-Perkins

Copy link
Copy Markdown
Contributor

copilot generated feedback

Found 4 correctness bugs and 2 structural/performance concerns. I excluded intentionally unsupported GIC functionality.

  1. High — IRQ entry corrupts SIMD/FP state
     tmk/tmk_core/src/aarch64.rs:119-155  saves GPRs but not  q0-q31 ,  FPCR , or  FPSR  before calling Rust. An asynchronous IRQ can corrupt interrupted SIMD state. Preserve the full FP/SIMD context or enforce a general-register-only handler ABI.
  2. High — repeated MSI interrupts are lost
     vmm_core/virt_support_gic/src/lib.rs:242-255  only latches edge interrupts on a false→true transition, while  vmm_core/virt/src/aarch64/gic_software_device.rs:67-71  sends every MSI as  set_spi_irq(..., true)  without deasserting. Only the first MSI is delivered. Add an explicit pulse API or treat each edge-triggered high request as a pulse.
  3. High — re-pending an active LR drops the interrupt
     openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:321-339  considers any existing LR sufficient. If it is active, the new SGI/timer request is subsequently removed at lines 850-866 instead of transitioning the LR to active-and-pending.
  4. High — SPI selection/reservation races across VPs
     vmm_core/virt_support_gic/src/lib.rs:322-391  selects under the distributor lock, but reservation occurs later through  mark_spi_injected  at  cca/mod.rs:891-915 . Multiple VPs can select the same IRM-routed SPI. Selection and reservation should be one atomic operation with rollback if no LR is available.
  5. Medium — hot-path work scales poorly
     retain_in_flight_spis  scans every configured INTID, and each LR fill repeats another full pending scan under the shared mutex ( virt_support_gic/src/lib.rs:268-391 ). Track the at-most-16 in-flight LRs per VP and maintain summary/ready bitmaps rather than scanning the full distributor repeatedly.
  6. Medium — private pending state uses the wrong representation
     cca/mod.rs:147-188  stores 32 possible private INTIDs in sixteen  Option  slots. This adds linear searches and can fatally reject a valid seventeenth pending private interrupt at lines 823-832. A  u32  pending bitmap directly represents GIC semantics and removes the artificial capacity limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.

Comment thread openhcl/virt_mshv_vtl/src/processor/cca/mod.rs Outdated
Comment thread vmm_core/virt_support_gic/src/lib.rs Outdated
let Some(gicr) = self.redistributors.get(vp.index() as usize) else {
return false;
};
let mut gicr = gicr.lock().expect("redistributor mutex error");
Comment on lines +715 to +717
let running_priority = running_priority(&self.runner.cca_rsi_plane_entry().gicv3_lrs);
let Some(interrupt) = self.shared.cvm.gic.next_pending_private_interrupt(self.vp_index(), running_priority)
else {
@chris-oo

Copy link
Copy Markdown
Member

there's another gicv3 PR out: #4319, how much of this is duplicated between both of them?

@chris-oo Chris Oo (chris-oo) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I need to spend a bit more time on GIC specifics but here's some first round feedback.

Comment thread openhcl/virt_mshv_vtl/src/processor/cca/mod.rs
Comment thread openhcl/virt_mshv_vtl/src/processor/cca/mod.rs Outdated
Comment thread openhcl/virt_mshv_vtl/src/processor/cca/mod.rs Outdated
Comment thread openhcl/virt_mshv_vtl/src/processor/cca/mod.rs Outdated
Comment thread openhcl/virt_mshv_vtl/src/processor/cca/mod.rs
Comment thread openhcl/virt_mshv_vtl/src/processor/cca/mod.rs
Comment thread openhcl/virt_mshv_vtl/src/processor/cca/mod.rs Outdated
Comment thread vmm_core/virt_support_gic/src/lib.rs Outdated
Comment thread vmm_core/virt_support_gic/src/lib.rs
Comment thread vmm_core/virt_support_gic/src/lib.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The CCA sysreg/priority-mask path described in the PR (ICC_PMR_EL1 handling and PMR-based filtering) is not actually implemented in the current code, which can lead to incorrect interrupt masking and/or unsupported traps terminating the VP.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

openhcl/virt_mshv_vtl/src/processor/cca/mod.rs:737

  • poll_gic uses running_priority(&...gicv3_lrs) as the priority filter passed into the GIC model. That value reflects the lowest active LR priority (preemption), not the guest-programmed ICC_PMR_EL1 priority mask. Without separately tracking PMR and applying it here, interrupt injection won’t respect the guest’s priority threshold as described (and may inject interrupts that should be masked).
            let running_priority = running_priority(&self.runner.cca_rsi_plane_entry().gicv3_lrs);
  • Files reviewed: 20/21 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread openhcl/virt_mshv_vtl/src/processor/cca/mod.rs Outdated
Add the initial ARM GICv3 interrupt-delivery framework for CCA lower
planes. Route virtual timer PPIs, self-generated SGIs, and shared SPIs
through a partition-owned GIC model and inject eligible interrupts
through the RMM GIC list registers.

Previously, CCA IRQ exits were only logged, trapped GIC accesses did not
provide a complete interrupt path, and local interrupt sources could be
inserted into an LR without respecting the guest-programmed GIC enable
and priority state.

CCA interrupt injection
-----------------------

Add a common CCA virtual-interrupt representation containing an INTID,
priority, group, and pending state.

Preserve the GIC list registers returned by the RMM and inject pending
software-backed Group 1 interrupts into the first free LR. Avoid adding
a duplicate LR when the same INTID is already pending or active.

Enable ICH_HCR_EL2.TC for plane entry and encode software LRs with:

* the virtual INTID in bits 31:0
* the guest-programmed priority in bits 55:48
* Group 1 in bit 60
* HW clear in bit 61
* Pending state in bits 63:62

Leave EOI-maintenance disabled because the CCA backend does not yet
implement the GIC maintenance-interrupt path.

Use an invalid exit-reason sentinel to distinguish a real plane exit
from a host wakeup, and preserve the RMM-returned plane and LR context
before processing the exit.

Virtual timer PPI
-----------------

Handle PlaneExitReason::Irq and recognize an asserted architectural
virtual timer from the RMM-provided CNTV_CTL_EL0 state.

Request the timer PPI only when:

* ENABLE is set
* IMASK is clear
* ISTATUS is set

Obtain the virtual timer INTID from ProcessorTopology instead of
hardcoding PPI 20. This is required because the RMM timer exit reports
the timer source but does not provide an INTID.

Queue the timer as a private interrupt and pass it through the shared
GIC redistributor selection path before LR injection. Delivery therefore
now observes:

* distributor Group 1 enable
* GICR_IGROUPR0
* GICR_ISENABLER0
* GICR active state
* the PPI priority programmed through GICR_IPRIORITYR
* the priority threshold programmed through ICC_PMR_EL1

Decode trapped ICC_PMR_EL1 writes and retain the current priority mask
for each CCA VTL. This fixes timer PPIs bypassing guest GIC enable and
priority configuration.

SGI
---

Decode synchronous ICC_SGI1R_EL1 system-register traps and queue the
requested self-SGI rather than injecting it directly.

Advance the lower-plane PC after emulating ICC_SGI1R_EL1 and
ICC_PMR_EL1 writes so that the trapped instruction is not executed
again after re-entry.

Route queued SGIs through the same private-interrupt selection used by
PPIs. SGI delivery therefore also observes the redistributor enable,
Group 1, active, programmed priority, and ICC_PMR_EL1 state.

Add a TMK self-SGI test using INTID 5 and the Group 1 system-register
interface.

SPI
---

Create a partition-owned GicV3Model for CCA and expose it to the VP
runner so that device interrupt assertion, CCA interrupt polling, and
guest GIC MMIO emulation operate on the same state.

Implement the CCA ControlGic::set_spi_irq path. Record the device line
level, determine the target VPs from GICD_IROUTER state, and wake those
VPs through the interrupt-controller wake reason when a line is newly
asserted.

Track SPI state independently as:

* software pending state
* external line assertion level
* active state
* LR/in-flight ownership

Select a shared interrupt only when it is pending or asserted, enabled,
assigned to Group 1, inactive, routed to the current VP, not already in
flight, and has a priority that passes ICC_PMR_EL1.

Preserve an edge-triggered SPI request after the source line falls by
latching it in the pending state until successful LR injection.

Preserve the asserted state of a level-triggered SPI after injection.
When the RMM retires its LR, clear only the in-flight ownership. If the
device line remains asserted, make the SPI eligible for injection again.
Stop redelivery only after the device deasserts the line.

This fixes level-triggered SPIs being lost after their first injection
and EOI.

Support guest software pending and clearing through GICD_ISPENDR and
GICD_ICPENDR. Deasserting a device line does not clear an independently
created software-pending request.

GIC model and TMK support
-------------------------

Extend the reusable GICv3 model with the distributor and redistributor
state needed by the CCA paths, including:

* Group 1 distributor enable
* interrupt group and enable state
* pending, active, and trigger configuration
* per-INTID priorities
* SPI routing
* GICR SGI/PPI enable, group, pending, active, and priority state
* priority-based private and shared interrupt selection
* MMIO reads and writes for the supported GICD/GICR registers

Define a common TMK AArch64 platform layout:

* GICD at 0xff00_0000
* GICR at 0xff02_0000
* virtual timer PPI 20
* 256 exposed INTIDs: SGI 0-15, PPI 16-31, and SPI 32-255

Add minimal EL1 IRQ support to tmk_core:

* install an aligned VBAR_EL1 exception-vector table
* save and restore registers around IRQ handling
* provide scoped IRQ callback registration
* configure GIC Group 1, interrupt enable, and priority state
* program ICC_SRE_EL1, ICC_PMR_EL1, and ICC_IGRPEN1_EL1
* acknowledge through ICC_IAR1_EL1
* complete through ICC_EOIR1_EL1
* mask and unmask IRQs through DAIF
* provide virtual timer, self-SGI, and software-pending SPI helpers

Route TMK GIC MMIO accesses to the same partition GicV3Model used by
the CCA VP. Add tests for:

* architectural virtual timer PPI delivery
* self-targeted SGI delivery
* software-pended SPI 32 delivery
* level-triggered SPI redelivery until deassertion
* edge-triggered SPI pending latching
* preservation of software-pending state across line deassertion
* SGI/PPI enable and priority filtering

Correct the RSI plane-exit layout so the timer state starts at its
ABI-defined 0x400 offset and assert the resulting structure offsets
and sizes.

Known limitations
-----------------

This is an initial CCA IRQ implementation, not a complete GICv3
implementation.

* PlaneExitReason::Irq currently recognizes only the architectural
  virtual timer. PMU, physical timer, and other local PPI sources are
  not identified or mapped.
* SGI support is limited to the single-VP/self-target test path. Full
  ICC_SGI1R_EL1 affinity routing, target lists, broadcast behavior, and
  multi-VP delivery are not implemented or validated.
* The SPI end-to-end test uses GICD_ISPENDR to create a software-pending
  SPI. It does not validate a real emulated device asserting and
  deasserting ControlGic::set_spi_irq through the complete device/VMM/
  Realm path.
* Level- and edge-triggered device SPI behavior is covered by GIC model
  unit tests, but still needs end-to-end device validation, including
  routing, VP wakeup, EOI/LR retirement, and repeated level delivery.
* The TMK platform currently creates one VP and exposes only INTIDs
  0-255. INTIDs 256-1019 are not implemented by this test platform.
* Only the GIC register subset required by the current tests is
  implemented. Group 0, complete security-state behavior, ITS/MSI/LPI,
  and full architectural GICv3 semantics are not supported.
* LR allocation is first-free. There is no LR eviction, maintenance
  interrupt handling, or complete priority-preemption model.
* Private pending storage is bounded, and running-priority, binary-point,
  and nested-interrupt behavior are not fully modeled.
* GIC EOI-maintenance events, cross-VTL interrupts, and the CCA
  untrusted-SynIC interrupt path remain unimplemented.
* MMIO polling in the TMK tests is a workaround for current RMM/FVP
  notification behavior rather than the final asynchronous delivery
  mechanism.
* Unsupported CCA IRQ sources are traced and ignored, while unsupported
  system-register traps terminate the VP.
* The current CCA backend supports only a single auxiliary plane; multiple
  planes and switching between them are not yet supported.

IRQ tests
---------

Add TMK tests covering the initial CCA interrupt-delivery paths:

* Virtual timer PPI: program CNTV_CVAL_EL0, wait for virtual timer PPI 20,
  verify delivery through the GIC, and disable the timer in the IRQ handler.

* Self-targeted SGI: generate Group 1 SGI 5 through ICC_SGI1R_EL1 and verify
  that it is delivered to the current VP.

* Software-pended SPI: set SPI 32 pending through GICD_ISPENDR and verify
  delivery through the distributor and GIC list-register path.

Install the CCA emulation environment once with:

    cargo xflowey cca-tests --install-emu

Build and run the CCA runtime test, including these TMK IRQ tests, with:

    cargo xflowey cca-tests

To build the test artifacts without running the emulator, use:

    cargo xflowey cca-tests --build-only

Note taht "Software-pended SPI” is intentionally precise: this test does not
exercise a real emulated device asserting an SPI line.

Test results
------------

    INFO test: test passed, name: "aarch64::irq::virtual_timer_irq"
    INFO test: test passed, name: "aarch64::irq::spi_self_pending_irq"
    INFO test: test passed, name: "aarch64::irq::sgi_self_irq"

Signed-off-by: Wei Ding <b-weiding@microsoft.com>
Signed-off-by: Ben Aram <b-bearam@microsoft.com>
Signed-off-by: Jiong Wang <b-jiongwang@microsoft.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

It introduces substantial cross-cutting changes to CCA interrupt delivery (GIC model, LR injection, sysreg/MMIO emulation, wakeups, and new TMK IRQ infrastructure), which warrants careful human validation beyond automated review.

Review details
  • Files reviewed: 20/21 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment on lines +233 to +235
pub fn new(gicd_base: u64, gicr_range: MemoryRange, interrupt_count: u32) -> Self {
let n = interrupt_count.div_ceil(32) as usize;
assert!(n <= u32::BITS as usize);
Comment on lines +1553 to +1558
pub fn clear_pending(&self, intid: u32) {
debug_assert!(intid < 32);

self.pending
.fetch_and(!(1 << intid), Ordering::Relaxed);
}
Comment on lines +24 to +28
pub fn signal_msi_gicv3(&self, _devid: Option<u32>, _address: u64, data: u32) {
if SPI_RANGE.contains(&data) {
self.irqcon.pulse_spi_irq(data);
}
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

unsafe Related to unsafe code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants