virt_hvf: correct ARM64 ID, timer, and trap behavior

[alextnewman]

What (UPDATED)

Correct four ARM64 behaviors in the HVF backend:

• preserve HVF's ID-register baseline while advertising GICv3 and a 36-bit IPA range;
• replace the fixed 2 ms WFI poll and synthetic timer PPI with the guest's virtual-timer deadline;
• distinguish WFI from WFE, WFIT, and WFET;
• treat Rt == 31 as XZR in trapped system-register instructions.

In response to review, this revision uses OpenVMM's existing block_on_vp waker/yield contract. The backend-local actor, Loom model, vCPU recreation, and supporting caches are removed.

Why

The current backend overwrites whole ID registers, fabricates timer interrupts while polling WFI, treats every trapped WF* instruction as WFI, and interprets system-register Rt == 31 as SP instead of XZR.

The reduced change boots an AZL-3-based Linux VM with the current in-tree GIC. A dependent strict GIC stack also boots to multi-user/SSH in three consecutive runs. ID and timer changes alone do not boot that stack, and restoring the old XZR behavior reproduces the failure.

This is a standalone ARM64 correctness change and the foundation for the follow-up strict GICv3 work—not a new concurrency architecture.

Contracts

• Arm DDI 0601: ID-register fields, virtual timer, ESR_EL2.ISS.TI, and system-register Rt.
• Arm DDI 0487: WFI and generic-timer behavior.
• Apple Hypervisor.framework: hv_vcpu_run, hv_vcpus_exit, VTimer masking, and counter offset.

Testing

• check, clippy, rustdoc, and cargo xtask fmt
• 6 focused virt_hvf tests
• 47 virt_support_gic tests on the dependent stack
• standalone Linux VM boot with the current GIC
• three consecutive Linux VM boots with the strict GIC stack
• differential actor and XZR mutations

Scope

Three files in aarch64defs and virt_hvf. No GIC delivery changes, MBI/MSI, physical timer, PMU, expanded PSCI, reset/save-restore, MANA, or private Hyper-V hypercalls. No public documentation changes are needed.

[github-actions]

⚠️ 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]

Pull request overview

This PR refactors the macOS ARM64 HVF virtual-processor runtime to use an event-driven notification + parking protocol (combining hv_vcpus_exit cancellation with async Waker parking), and adds ARM64 architectural behavior modeling (ID-register policy, generic timer/WFI handling, PSCI subset, and a PMU-compatibility shim).

Changes:

• Introduces VpActor to coordinate publication/notification/parking and owning-thread-safe vCPU replacement/teardown.
• Refactors the run loop to remove fixed WFI polling and instead park with timer-deadline + interrupt-aware wakeups.
• Adds ARM64 architectural modeling: VM-wide ID-register policy, PSCI power states, counter sysregs, and a synthetic PMU cycle-counter model (plus tests and Loom model support).

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
vmm_core/virt_hvf/src/vp_actor.rs	Adds the new VpActor synchronization primitive with unit tests and Loom model coverage.
vmm_core/virt_hvf/src/lib.rs	Integrates VpActor into VP lifecycle and run loop; implements ID-register policy, PSCI subset, timer/WFI behavior, and PMU-compatibility model.
vmm_core/virt_hvf/src/abi.rs	Documents HVF contracts and adds missing ABI surface (vtimer offset + mach_absolute_time).
vmm_core/virt_hvf/Cargo.toml	Adds optional loom feature/dependency wiring for in-tree concurrency modeling.
vm/aarch64/aarch64defs/src/lib.rs	Adds system-register encodings for CNTPCT_EL0 and CNTVCT_EL0.
Cargo.lock	Records the new loom dependency inclusion.

Comments suppressed due to low confidence (2)

vmm_core/virt_hvf/src/lib.rs:1443

• Avoid panicking in HvfProcessor's Drop. A failed vCPU destroy/removal should not take down the whole VMM during teardown; log and continue instead of .expect(...).

        self.inner
            .actor
            .remove_vcpu(|| self.vcpu.destroy())
            .expect("vcpu destroy cannot fail");
    }

vmm_core/virt_hvf/src/lib.rs:1719

• This warning is guest-triggerable (unknown trapped system-register write) and can flood logs. Consider switching to tracelimit::warn_ratelimited! to avoid unbounded log spam from a misbehaving guest.

                                    tracing::warn!(
                                        ?reg,
                                        value,
                                        pc = self.vcpu.pc(),
                                        "ignoring write to unknown system register"

[Copilot]

Pull request overview

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

[github-actions]

3 Petri tests failed (2 unstable)

[jstarks]

Copilot assisted feedback:

Thanks for pushing on the ARM64 backend. The ID-register policy is a real improvement over what was there, and dropping the 2 ms WFI poll in favor of the architected virtual-timer deadline is exactly right. My concerns are with the supporting machinery — I think most of it can go, and the PR gets a lot smaller as a result.

1. VpActor duplicates the waker contract that run_vp is built on

run_vp is deliberately an async fn that's allowed to block the thread synchronously. What makes that safe lives on the waker side, not in the backend. Each VP thread runs its future under block_on_vp:

// openvmm/openvmm_core/src/worker/dispatch.rs
block_on_vp(partition, VpIndex::new(vp_index as u32), vp.run(runner, &chipset))

and block_on_vp wraps the thread's waker in VpWaker before every poll:

// vmm_core/src/partition_unit/vp_set.rs
impl std::task::Wake for VpWaker {
    fn wake_by_ref(self: &Arc<Self>) {
        self.partition.request_yield(self.vp);   // -> HvfPartition::request_yield
        self.inner.wake_by_ref();                // -> re-poll the VP task
    }
}

So the cx.waker() that HvfProcessor::run_vp receives already calls Partition::request_yield, which in this backend is needs_yield.request_yield() → cancel_run() → hv_vcpus_exit. Waking it means "kick this vCPU out of hv_vcpu_run and re-poll me." That's the whole point of the contract, and it's why the doc comment on run_vp says the future must return Poll::Pending once request_yield has been called.

Consequences:

• VpActor::notify() is cancel_run() + wake — which is what waking the stored waker already did. The pre-existing HvfVpInner::wake() wasn't "just a task wake"; it went through VpWaker and cancelled the run.
• The new path is a small regression. request_yield coalesces through NeedsYield, so a burst of synic messages or SPI assertions collapses into one hv_vcpus_exit. notify() issues one unconditionally per notification. And since run_vp still awaits needs_yield.maybe_yield() at the top of its loop, we now have two kick paths per vCPU, with the new one bypassing the coalescing the old one was designed around.
• The sequence/ScanToken/begin_scan/try_park protocol is solving a hazard the actor introduces. notify() takes the waker out of the mutex, so a notification landing between begin_scan() and try_park() finds None and wakes nobody — hence the generation recheck. Keep the waker registered (the will_wake-guarded re-store idiom the old code used, and that virt_whp::vp::run_vp still uses) and the executor's wake-during-poll latch covers that window for free: a wake() arriving mid-poll forces a re-poll, so publish-then-wake can't be lost. The counter, the token, ParkDecision, and the still_idle closures all fall out.
• The Loom model is therefore verifying our re-derivation of Waker semantics, and the loom dev-dependency comes with it.

The module doc says HVF gives a vCPU "two distinct wake paths" that need joining. They're already joined, one layer up, for every backend — and keeping that join in one place is worth more than a per-backend actor.

Suggested shape: delete vp_actor.rs; keep HvfVpInner::wake(); park the WFI and not-yet-powered-on cases by returning Poll::Pending with cx.waker() registered, the way virt_whp does. That keeps the real win here — parking on the architected vtimer deadline instead of polling — with no new machinery.

If you believe there's a hole in the VpWaker latch that I'm not seeing, let's nail it down concretely and fix it in vmm_core, so every backend benefits.

2. Skeptical of destroy + recreate on CPU_ON

I understand the motivation: this PR makes CPU_OFF a real transition (it used to return DENIED), so a subsequent CPU_ON must hand the guest an architecturally-reset vCPU, and Hypervisor.framework has no reset API. But:

• It's pure cost on the common path. A secondary's vCPU is created in bind and has never entered the guest; the first CPU_ON destroys it to create an identical one. Only a genuine off→on cycle needs anything.
• It's an incomplete reset anyway. power_on defaults self.pmu but leaves self.gicr untouched, so pending and active PPI state survives the cycle. We pay for a full vCPU teardown and still hand-reset some things while missing others.
• It's the root of most of the rest of this review — items 3 and 4 below both exist to service it.

Suggested alternative: extend the common ARM64 VP state model. virt::aarch64::vp::SystemRegisters is nine registers today — SCTLR_EL1, TTBR0/1_EL1, TCR_EL1, ESR_EL1, FAR_EL1, MAIR_EL1, ELR_EL1, VBAR_EL1. No CPACR_EL1, SPSR_EL1, TPIDR_EL0/TPIDR_EL1/TPIDRRO_EL0, CONTEXTIDR_EL1, AMAIR_EL1, MDSCR_EL1, CNTV_CTL_EL0/CNTV_CVAL_EL0. Filling that in buys three things at once:

1. set_reset_registers becomes a real reset, so recreate_vcpu goes away.
2. ARM64 save/restore stops silently dropping those registers — a latent gap across all ARM64 backends, not just HVF.
3. The Processor::reset work this PR defers gets much easier.

If there's state HVF exposes that we genuinely cannot write back, let's enumerate it in a comment. That would make the case for recreation concrete instead of precautionary.

3. Can we avoid the Mutex around the vCPU id entirely?

I'd rather we try to eliminate the need for Mutex<Option<u64>> + replace_vcpu/remove_vcpu than encode a lifecycle contract in a lock. Two routes:

(a) Never tear a vCPU down. If item 2 lands, the id is written once in bind and cleared on drop, and the only exposure is a teardown race. The actual bug in the old code was the .chk().unwrap() in cancel_run — a stale-id HV_BAD_ARGUMENT would panic whichever device or synic thread happened to be notifying. That's worth fixing regardless, and it's a two-line fix.

(b) If teardown has to exist, let's establish what HVF actually does with a stale hv_vcpu_t rather than assuming. It's a u64 handle, not a pointer, so I'd expect the framework to validate it and return HV_BAD_ARGUMENT — but neither of us should be guessing at this. Could you check Apple's documentation, and ideally confirm experimentally?

• If stale ids are reliably rejected, then "clear the id on destroy, don't unwrap the result" is sufficient and we need no lock. Even a recycled id delivering a cancellation to an unrelated vCPU is harmless — the run loop already treats HvExitReason::CANCELED as "loop and rescan," and spurious cancellations are assumed throughout this design.
• If they aren't reliably rejected — if a recycled id can do something worse than a spurious exit — that's important enough to document, and then the Mutex is justified and the comment should say so with a citation.

4. The ID-register cache looks like an artifact of items 2 and 3

id_register_policy(IdRegisters::read(&vcpu)) is a pure function of a value that's constant for the VM, so computing it independently in each bind would give identical results. The Mutex<Option<IdRegisters>>, the lazy initialization, and the .context("missing partition ID-register model") path exist because recreate_vcpu needs to re-install the model from a context where the original vCPU is gone.

The doc comment describes this as defining "one VM-wide virtual CPU model," which reads as though it's guarding against per-vCPU divergence. I don't think that hazard exists: Apple Silicon P and E cores must agree on ID registers (macOS migrates threads between them with no feature negotiation), these are HVF's software-maintained feature registers rather than live reads of the current physical core, and HVF exposes feature registers on the hv_vcpu_config object before any vCPU exists at all. The code also already assumes uniformity — it reads from whichever vCPU binds first and installs that everywhere without comparing.

So: drop the recreate path and this becomes either a plain immutable field computed once in HvfPartition::new (the third argument to hv_vcpu_create is a config we currently pass as null — can the baseline be read from one?), or nothing at all.

[alextnewman]

Copilot assisted feedback:

Thanks for pushing on the ARM64 backend. The ID-register policy is a real improvement over what was there, and dropping the 2 ms WFI poll in favor of the architected virtual-timer deadline is exactly right. My concerns are with the supporting machinery — I think most of it can go, and the PR gets a lot smaller as a result.

1. VpActor duplicates the waker contract that run_vp is built on

run_vp is deliberately an async fn that's allowed to block the thread synchronously. What makes that safe lives on the waker side, not in the backend. Each VP thread runs its future under block_on_vp:

// openvmm/openvmm_core/src/worker/dispatch.rs
block_on_vp(partition, VpIndex::new(vp_index as u32), vp.run(runner, &chipset))

and block_on_vp wraps the thread's waker in VpWaker before every poll:

// vmm_core/src/partition_unit/vp_set.rs
impl std::task::Wake for VpWaker {
    fn wake_by_ref(self: &Arc<Self>) {
        self.partition.request_yield(self.vp);   // -> HvfPartition::request_yield
        self.inner.wake_by_ref();                // -> re-poll the VP task
    }
}

So the cx.waker() that HvfProcessor::run_vp receives already calls Partition::request_yield, which in this backend is needs_yield.request_yield() → cancel_run() → hv_vcpus_exit. Waking it means "kick this vCPU out of hv_vcpu_run and re-poll me." That's the whole point of the contract, and it's why the doc comment on run_vp says the future must return Poll::Pending once request_yield has been called.

Consequences:

• VpActor::notify() is cancel_run() + wake — which is what waking the stored waker already did. The pre-existing HvfVpInner::wake() wasn't "just a task wake"; it went through VpWaker and cancelled the run.
• The new path is a small regression. request_yield coalesces through NeedsYield, so a burst of synic messages or SPI assertions collapses into one hv_vcpus_exit. notify() issues one unconditionally per notification. And since run_vp still awaits needs_yield.maybe_yield() at the top of its loop, we now have two kick paths per vCPU, with the new one bypassing the coalescing the old one was designed around.
• The sequence/ScanToken/begin_scan/try_park protocol is solving a hazard the actor introduces. notify() takes the waker out of the mutex, so a notification landing between begin_scan() and try_park() finds None and wakes nobody — hence the generation recheck. Keep the waker registered (the will_wake-guarded re-store idiom the old code used, and that virt_whp::vp::run_vp still uses) and the executor's wake-during-poll latch covers that window for free: a wake() arriving mid-poll forces a re-poll, so publish-then-wake can't be lost. The counter, the token, ParkDecision, and the still_idle closures all fall out.
• The Loom model is therefore verifying our re-derivation of Waker semantics, and the loom dev-dependency comes with it.

The module doc says HVF gives a vCPU "two distinct wake paths" that need joining. They're already joined, one layer up, for every backend — and keeping that join in one place is worth more than a per-backend actor.

Suggested shape: delete vp_actor.rs; keep HvfVpInner::wake(); park the WFI and not-yet-powered-on cases by returning Poll::Pending with cx.waker() registered, the way virt_whp does. That keeps the real win here — parking on the architected vtimer deadline instead of polling — with no new machinery.

If you believe there's a hole in the VpWaker latch that I'm not seeing, let's nail it down concretely and fix it in vmm_core, so every backend benefits.

2. Skeptical of destroy + recreate on CPU_ON

I understand the motivation: this PR makes CPU_OFF a real transition (it used to return DENIED), so a subsequent CPU_ON must hand the guest an architecturally-reset vCPU, and Hypervisor.framework has no reset API. But:

• It's pure cost on the common path. A secondary's vCPU is created in bind and has never entered the guest; the first CPU_ON destroys it to create an identical one. Only a genuine off→on cycle needs anything.
• It's an incomplete reset anyway. power_on defaults self.pmu but leaves self.gicr untouched, so pending and active PPI state survives the cycle. We pay for a full vCPU teardown and still hand-reset some things while missing others.
• It's the root of most of the rest of this review — items 3 and 4 below both exist to service it.

Suggested alternative: extend the common ARM64 VP state model. virt::aarch64::vp::SystemRegisters is nine registers today — SCTLR_EL1, TTBR0/1_EL1, TCR_EL1, ESR_EL1, FAR_EL1, MAIR_EL1, ELR_EL1, VBAR_EL1. No CPACR_EL1, SPSR_EL1, TPIDR_EL0/TPIDR_EL1/TPIDRRO_EL0, CONTEXTIDR_EL1, AMAIR_EL1, MDSCR_EL1, CNTV_CTL_EL0/CNTV_CVAL_EL0. Filling that in buys three things at once:

1. set_reset_registers becomes a real reset, so recreate_vcpu goes away.
2. ARM64 save/restore stops silently dropping those registers — a latent gap across all ARM64 backends, not just HVF.
3. The Processor::reset work this PR defers gets much easier.

If there's state HVF exposes that we genuinely cannot write back, let's enumerate it in a comment. That would make the case for recreation concrete instead of precautionary.

3. Can we avoid the Mutex around the vCPU id entirely?

I'd rather we try to eliminate the need for Mutex<Option<u64>> + replace_vcpu/remove_vcpu than encode a lifecycle contract in a lock. Two routes:

(a) Never tear a vCPU down. If item 2 lands, the id is written once in bind and cleared on drop, and the only exposure is a teardown race. The actual bug in the old code was the .chk().unwrap() in cancel_run — a stale-id HV_BAD_ARGUMENT would panic whichever device or synic thread happened to be notifying. That's worth fixing regardless, and it's a two-line fix.

(b) If teardown has to exist, let's establish what HVF actually does with a stale hv_vcpu_t rather than assuming. It's a u64 handle, not a pointer, so I'd expect the framework to validate it and return HV_BAD_ARGUMENT — but neither of us should be guessing at this. Could you check Apple's documentation, and ideally confirm experimentally?

• If stale ids are reliably rejected, then "clear the id on destroy, don't unwrap the result" is sufficient and we need no lock. Even a recycled id delivering a cancellation to an unrelated vCPU is harmless — the run loop already treats HvExitReason::CANCELED as "loop and rescan," and spurious cancellations are assumed throughout this design.
• If they aren't reliably rejected — if a recycled id can do something worse than a spurious exit — that's important enough to document, and then the Mutex is justified and the comment should say so with a citation.

4. The ID-register cache looks like an artifact of items 2 and 3

id_register_policy(IdRegisters::read(&vcpu)) is a pure function of a value that's constant for the VM, so computing it independently in each bind would give identical results. The Mutex<Option<IdRegisters>>, the lazy initialization, and the .context("missing partition ID-register model") path exist because recreate_vcpu needs to re-install the model from a context where the original vCPU is gone.

The doc comment describes this as defining "one VM-wide virtual CPU model," which reads as though it's guarding against per-vCPU divergence. I don't think that hazard exists: Apple Silicon P and E cores must agree on ID registers (macOS migrates threads between them with no feature negotiation), these are HVF's software-maintained feature registers rather than live reads of the current physical core, and HVF exposes feature registers on the hv_vcpu_config object before any vCPU exists at all. The code also already assumes uniformity — it reads from whichever vCPU binds first and installs that everywhere without comparing.

So: drop the recreate path and this becomes either a plain immutable field computed once in HvfPartition::new (the third argument to hv_vcpu_create is a config we currently pass as null — can the baseline be read from one?), or nothing at all.

Thanks for the feedback. I broke out the concurrency model to test a lot of subtle and broken behaviors and that out-of-tree reinvention probably needed to be reconciled once the model with hvf was shaken out. I intentionally avoided that to keep hvf changes isolated and it was probably too timid, I'll attempt a flatter, more surgical fix to the core.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[alextnewman]

Copilot assisted feedback:

Thanks for pushing on the ARM64 backend. The ID-register policy is a real improvement over what was there, and dropping the 2 ms WFI poll in favor of the architected virtual-timer deadline is exactly right. My concerns are with the supporting machinery — I think most of it can go, and the PR gets a lot smaller as a result.

1. VpActor duplicates the waker contract that run_vp is built on

run_vp is deliberately an async fn that's allowed to block the thread synchronously. What makes that safe lives on the waker side, not in the backend. Each VP thread runs its future under block_on_vp:

// openvmm/openvmm_core/src/worker/dispatch.rs
block_on_vp(partition, VpIndex::new(vp_index as u32), vp.run(runner, &chipset))

and block_on_vp wraps the thread's waker in VpWaker before every poll:

// vmm_core/src/partition_unit/vp_set.rs
impl std::task::Wake for VpWaker {
    fn wake_by_ref(self: &Arc<Self>) {
        self.partition.request_yield(self.vp);   // -> HvfPartition::request_yield
        self.inner.wake_by_ref();                // -> re-poll the VP task
    }
}

So the cx.waker() that HvfProcessor::run_vp receives already calls Partition::request_yield, which in this backend is needs_yield.request_yield() → cancel_run() → hv_vcpus_exit. Waking it means "kick this vCPU out of hv_vcpu_run and re-poll me." That's the whole point of the contract, and it's why the doc comment on run_vp says the future must return Poll::Pending once request_yield has been called.

Consequences:

• VpActor::notify() is cancel_run() + wake — which is what waking the stored waker already did. The pre-existing HvfVpInner::wake() wasn't "just a task wake"; it went through VpWaker and cancelled the run.
• The new path is a small regression. request_yield coalesces through NeedsYield, so a burst of synic messages or SPI assertions collapses into one hv_vcpus_exit. notify() issues one unconditionally per notification. And since run_vp still awaits needs_yield.maybe_yield() at the top of its loop, we now have two kick paths per vCPU, with the new one bypassing the coalescing the old one was designed around.
• The sequence/ScanToken/begin_scan/try_park protocol is solving a hazard the actor introduces. notify() takes the waker out of the mutex, so a notification landing between begin_scan() and try_park() finds None and wakes nobody — hence the generation recheck. Keep the waker registered (the will_wake-guarded re-store idiom the old code used, and that virt_whp::vp::run_vp still uses) and the executor's wake-during-poll latch covers that window for free: a wake() arriving mid-poll forces a re-poll, so publish-then-wake can't be lost. The counter, the token, ParkDecision, and the still_idle closures all fall out.
• The Loom model is therefore verifying our re-derivation of Waker semantics, and the loom dev-dependency comes with it.

The module doc says HVF gives a vCPU "two distinct wake paths" that need joining. They're already joined, one layer up, for every backend — and keeping that join in one place is worth more than a per-backend actor.

Suggested shape: delete vp_actor.rs; keep HvfVpInner::wake(); park the WFI and not-yet-powered-on cases by returning Poll::Pending with cx.waker() registered, the way virt_whp does. That keeps the real win here — parking on the architected vtimer deadline instead of polling — with no new machinery.

If you believe there's a hole in the VpWaker latch that I'm not seeing, let's nail it down concretely and fix it in vmm_core, so every backend benefits.

2. Skeptical of destroy + recreate on CPU_ON

I understand the motivation: this PR makes CPU_OFF a real transition (it used to return DENIED), so a subsequent CPU_ON must hand the guest an architecturally-reset vCPU, and Hypervisor.framework has no reset API. But:

• It's pure cost on the common path. A secondary's vCPU is created in bind and has never entered the guest; the first CPU_ON destroys it to create an identical one. Only a genuine off→on cycle needs anything.
• It's an incomplete reset anyway. power_on defaults self.pmu but leaves self.gicr untouched, so pending and active PPI state survives the cycle. We pay for a full vCPU teardown and still hand-reset some things while missing others.
• It's the root of most of the rest of this review — items 3 and 4 below both exist to service it.

Suggested alternative: extend the common ARM64 VP state model. virt::aarch64::vp::SystemRegisters is nine registers today — SCTLR_EL1, TTBR0/1_EL1, TCR_EL1, ESR_EL1, FAR_EL1, MAIR_EL1, ELR_EL1, VBAR_EL1. No CPACR_EL1, SPSR_EL1, TPIDR_EL0/TPIDR_EL1/TPIDRRO_EL0, CONTEXTIDR_EL1, AMAIR_EL1, MDSCR_EL1, CNTV_CTL_EL0/CNTV_CVAL_EL0. Filling that in buys three things at once:

1. set_reset_registers becomes a real reset, so recreate_vcpu goes away.
2. ARM64 save/restore stops silently dropping those registers — a latent gap across all ARM64 backends, not just HVF.
3. The Processor::reset work this PR defers gets much easier.

If there's state HVF exposes that we genuinely cannot write back, let's enumerate it in a comment. That would make the case for recreation concrete instead of precautionary.

3. Can we avoid the Mutex around the vCPU id entirely?

I'd rather we try to eliminate the need for Mutex<Option<u64>> + replace_vcpu/remove_vcpu than encode a lifecycle contract in a lock. Two routes:

(a) Never tear a vCPU down. If item 2 lands, the id is written once in bind and cleared on drop, and the only exposure is a teardown race. The actual bug in the old code was the .chk().unwrap() in cancel_run — a stale-id HV_BAD_ARGUMENT would panic whichever device or synic thread happened to be notifying. That's worth fixing regardless, and it's a two-line fix.

(b) If teardown has to exist, let's establish what HVF actually does with a stale hv_vcpu_t rather than assuming. It's a u64 handle, not a pointer, so I'd expect the framework to validate it and return HV_BAD_ARGUMENT — but neither of us should be guessing at this. Could you check Apple's documentation, and ideally confirm experimentally?

• If stale ids are reliably rejected, then "clear the id on destroy, don't unwrap the result" is sufficient and we need no lock. Even a recycled id delivering a cancellation to an unrelated vCPU is harmless — the run loop already treats HvExitReason::CANCELED as "loop and rescan," and spurious cancellations are assumed throughout this design.
• If they aren't reliably rejected — if a recycled id can do something worse than a spurious exit — that's important enough to document, and then the Mutex is justified and the comment should say so with a citation.

4. The ID-register cache looks like an artifact of items 2 and 3

id_register_policy(IdRegisters::read(&vcpu)) is a pure function of a value that's constant for the VM, so computing it independently in each bind would give identical results. The Mutex<Option<IdRegisters>>, the lazy initialization, and the .context("missing partition ID-register model") path exist because recreate_vcpu needs to re-install the model from a context where the original vCPU is gone.

The doc comment describes this as defining "one VM-wide virtual CPU model," which reads as though it's guarding against per-vCPU divergence. I don't think that hazard exists: Apple Silicon P and E cores must agree on ID registers (macOS migrates threads between them with no feature negotiation), these are HVF's software-maintained feature registers rather than live reads of the current physical core, and HVF exposes feature registers on the hv_vcpu_config object before any vCPU exists at all. The code also already assumes uniformity — it reads from whichever vCPU binds first and installs that everywhere without comparing.

So: drop the recreate path and this becomes either a plain immutable field computed once in HvfPartition::new (the third argument to hv_vcpu_create is a config we currently pass as null — can the baseline be read from one?), or nothing at all.

Confirmed that this was bring-up cruft that was conflated with actual fixes, probably introduced while debugging what looked like concurrency issues but ended up being more surface-level.

I removed VpActor, Loom, vCPU recreation, and the supporting caches, then redid the boundary from differential guest evidence. The PR is now a three-file ARM64 correctness change: truthful ID/IPA policy, architected virtual-timer WFI behavior, WF* decoding, and load-bearing system-register XZR semantics. The standalone change boots with the current in-tree GIC, and the exact dependent strict-GIC stack boots a Linux VM to multi-user/SSH in three consecutive runs. PMU, expanded PSCI, and reset remain separate follow-up work.

[Copilot]

Pull request overview

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

[github-actions]

1 Petri tests failed (0 unstable)

[jstarks]

Why only 36? This is pretty limiting in terms of physical address space.

[alextnewman]

This was a conservative floor that (probably pointlessly) preserved macOS 11 support, where there's no hv_vm_config_get_default_ipa_size(). macOS 13+ is a more than reasonable floor, so I can just make it dynamic.

[alextnewman]

For some additional context, 36 bits is default in hvf, so the choice wasn't arbitrary. Running that locally on my M5:

default IPA size: 36 bits (64 GiB)
maximum IPA size: 40 bits (1 TiB)

[alextnewman]

So, 36-bits is the boundary where 16k stage-2 translation avoids another page level. Given macOS is a dev/workstation platform, it would be rare for a VM to be above 64GiB, so I think the efficiency win is probably correct (we query for default and use that). I think an adaptive model where we balloon to max when we request a VM north of that makes sense, but I'm not sure if that makes sense in the hvf leaf.

[alextnewman]

Fixed in cc835b2

[jstarks]

Out of curiousity, why have a cap at all?

[alextnewman]

(per the model...) The cap protects the host-time conversion rather than limiting the guest timer. A guest can program CNTV_CVAL_EL0 arbitrarily far into the future, but VmTime eventually converts that distance into a host Instant, whose representable range is smaller and may panic on overflow.

At the cap, the VP simply wakes and recomputes the same architectural deadline; no guest interrupt is injected and the guest-visible compare value is unchanged. I will rename this MAX_HOST_TIMER_WAIT and clarify that it is a bounded host recheck interval, not an architectural timer limit.

So. It's host protection from dangerous time travelers.

[alextnewman]

Renamed and clarified in cc835b2

[Copilot]

🟡 Not ready to approve

It introduces a macOS-13+ HVF symbol without an availability strategy and still has a guest-triggerable unwrap() on an HVF call in the vCPU run loop.

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

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Human review recommended

The changes affect low-level ARM64 CPU model/timer/trap semantics in the HVF backend, which are correctness-critical and warrant final human review despite looking internally consistent.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.