Fix issues with vGIC LR management - #334
Conversation
|
Hey @rabara! Having you aknowledge this PR fixes issues on your side also is already another push to merging this. Last week I asked @DavidMCerdeira that he makes this a high priority. So I am hoping we merge at most in a couple weeks. Hopefully, less. |
DavidMCerdeira
left a comment
DavidMCerdeira
left a comment
There was a problem hiding this comment.
In commit 2cc772b:
HW PENDACT is impossible (the physical GIC prevents re-assertion
while active), so state & PEND is always 0 for HW.
I don't think this is true, can you explain a bit more?
In commit db718b8
When all LRs are full and pend_found > 1 but all pending LRs have higher
priority than the new interrupt (pend_ind == -1), the spill selection set
lr_ind = -1 and skipped eviction entirely, even if a lower-priority active
LR was available to spill.
Add pend_ind >= 0 to the condition so the selection falls through to
act_ind when no eligible pending candidate exists.
I think it should be:
When all LRs are full, the spill selection set lr_ind = -1 and skipped eviction entirely, even if a lower-priority active LR was available to spill.
Add pend_ind >= 0 to the condition so the selection falls through to
act_ind when no eligible pending candidate exists.
| * track it in the spilled list so it gets injected when an LR slot | ||
| * becomes available. | ||
| */ | ||
| if (!interrupt->in_lr && !interrupt->in_spilled) { |
There was a problem hiding this comment.
This check is already performed in vgic_add_spilled, why check it outside as well?
If it's for performance reasons a comment should state that.
The comment explaining the current check should be moved to vgic_add_spilled.
There was a problem hiding this comment.
The two checks have different purposes. The one in vgic_route is a routing decision: the interrupt was not placed in an LR and was not handed to another CPU, so this core keeps tracking it in the spilled list. The one inside vgic_add_spilled is a correctness guard so the function is safe to call from any context, since an interrupt that is in an LR or already listed must never be pushed again.
That said, while going through this, I noticed the routing condition did not actually match its comment: the forwarded case was never checked, so an interrupt forwarded to another CPU was still added to the spilled list after ownership was yielded, and a refill on a non-target vcpu could then pick it up. Fixed in 5259930: vgic_route now skips the fallback for forwarded interrupts, the comment moved into vgic_add_spilled, and NPIE is only armed when an interrupt is actually queued.
| spin_unlock(&vcpu->vm->arch.vgic_spilled_lock); | ||
| gich_set_hcr(gich_get_hcr() | GICH_HCR_NPIE_BIT); | ||
| struct list* spilled_list = gic_is_priv(interrupt->id) | ||
| ? &vcpu->arch.vgic_spilled |
There was a problem hiding this comment.
There's something I'd like to make clear here: there's only a lock for the SPI spilled list, vcpu.vm.arch.vgic_spilled, but no lock for the private interrupts list, vcpu.arch.gic_spilled
It might be worth explaining this in a comment
There was a problem hiding this comment.
There is actually no need for a lock on the private list: it is only ever touched by the cpu hosting the vcpu those interrupts are private to. Accesses from other cores to another vcpu's private interrupts go through the vgic IPI, so they also end up running on the hosting cpu.
|
I finally got some time to review this. In addition to my other comments, @josecm please update the branch and fix the issues identified by the gitlint and the format check. |
You are right, this is not true in general. It only holds for the physical delivery path: since the hypervisor only does the priority drop and leaves deactivation to the guest, the physical interrupt stays active while the guest handles it, and the GIC does not signal an interrupt that is still active. A re-assertion just parks as physical pending, so the hypervisor never gets to inject pend on top of a virtual act. But the guest can still produce pend and act on a hw interrupt through the virtual distributor: an ISPENDR write while the interrupt is active, or an ISACTIVER write while it is pending, goes through vgic_int_update_pend/vgic_int_update_act and ends up with both bits set. That is actually why vgic_write_lr already had the special case writing only ACT for a PENDACT hw interrupt, as an LR with the HW bit set cannot hold the pending and active state. The code change itself does not depend on the wrong claim: the hw branch needs no strip because the LR is written with only ACT and the pending state is tracked by the physical GIC, which will not re-deliver it while the interrupt is disabled given the physical enable mirrors the virtual one. I will reword the commit message accordingly when I update the branch. |
That would overstate the bug. With all LRs full the old code only skipped eviction in one specific case: more than one LR holding a pending interrupt, but none of them an eligible victim. If an eligible pending LR existed it was spilled, and with at most one pending LR the selection already fell through to the active candidate. So the "more than one pending, none eligible" condition is what the bug actually requires and I would rather keep it in the message. I agree the current phrasing leans too much on code internals though. When I update the branch I will reword it without the variable names, to something like: "When all LRs are full and more than one of them holds a pending interrupt, but none of those is an eligible victim (none has lower priority than the new interrupt), the spill selection skipped eviction entirely, even if a lower priority active LR was available to spill." |
When a guest disables an interrupt that is currently active in an LR, vgic_route() and vgic_add_lr() were returning early due to the !interrupt->enabled check, leaving the interrupt untracked. On re-enable, Bao would re-inject it as active causing incorrect state. Allow disabled-but-active interrupts through so the guest can EOI them directly via the LR, avoiding a spurious LRENP maintenance interrupt trap. Signed-off-by: Jose Martins <josemartins90@gmail.com>
vgic_eoir_highest_spilled_active() was clearing the ACT state of the highest-priority spilled active interrupt but never removing it from the spilled list, causing it to be re-processed on subsequent maintenance interrupts or LR refills. Add list_rm() after gaining ownership. vgic_spilled_lock is held around the search and removal; it is released before vgic_add_lr() to avoid a deadlock with vgic_add_spilled() on the SW+PEND path. Signed-off-by: Jose Martins <josemartins90@gmail.com>
When vgic_route() fails to place an interrupt in an LR or forward it to another CPU (e.g. no target configured), the interrupt was left untracked — not in any LR and not in the spilled list — causing it to be missed on future LR refills. Add in_spilled flag to vgic_int to prevent double-adds. vgic_add_spilled() guards on !in_lr && !in_spilled, and clears/sets the flag alongside list operations. vgic_route() unconditionally calls vgic_add_spilled() as a fallback after routing, which is a no-op if the interrupt was already placed in an LR or spilled by vgic_add_lr(). Signed-off-by: Jose Martins <josemartins90@gmail.com>
When vgic_add_lr() successfully placed an interrupt into an LR, it left a stale entry in the spilled list, causing vgic_refill_lrs() to find and attempt to re-process an already-tracked interrupt. Introduce vgic_remove_spilled() to wrap list_rm() and the in_spilled flag update in one place. Call it in vgic_add_lr() before vgic_write_lr(), and replace the open-coded equivalents in vgic_refill_lrs() and vgic_eoir_highest_spilled_active(). Drop the now-unused outlist parameter from vgic_highest_prio_spilled(). Signed-off-by: Jose Martins <josemartins90@gmail.com>
Sending an IPI for an active interrupt is pointless: vgic_yield_ownership() refuses to yield ownership while the interrupt is active, so the receiving CPU will fail to acquire ownership and do nothing. Avoid the unnecessary IPI by guarding the forwarding path with !(interrupt->state & ACT). Signed-off-by: Jose Martins <josemartins90@gmail.com>
When a disabled PENDACT interrupt is placed in an LR, the LR was written with both ACT and PEND set. On guest EOI the LR would transition from PENDACT to PEND and re-deliver the interrupt even though it is disabled. Strip PEND from the LR state written for disabled non-HW interrupts, and preserve it in interrupt->state so it is restored when the interrupt is re-enabled. HW interrupts need no equivalent handling: a PENDACT HW interrupt already has its LR written with only ACT, as an LR with the HW bit set cannot hold the pending and active state, and its pending state is instead tracked by the physical GIC, which cannot re-deliver it while the interrupt is disabled since the physical enable mirrors the virtual one. Also fix vgic_remove_lr() to OR in the preserved PEND when updating interrupt->state from the LR, so that an early removal (e.g. on re-enable before the guest EOIs) does not overwrite the preserved pending state. Signed-off-by: Jose Martins <josemartins90@gmail.com>
When all LRs are full and more than one of them holds a pending interrupt, but none of those is an eligible victim (none has lower priority than the new interrupt), the spill selection skipped eviction entirely, even if a lower priority active LR was available to spill. Add pend_ind >= 0 to the condition so the selection falls through to the active candidate when no eligible pending candidate exists. Signed-off-by: Jose Martins <josemartins90@gmail.com>
When vgic_route() forwards an interrupt to another CPU it also yields ownership, so adding it to the spilled list as well would let a refill on a non-target vcpu take ownership and inject it there. Skip the spilled list fallback when the interrupt was forwarded, and arm NPIE only when an interrupt is actually queued. Signed-off-by: Jose Martins <josemartins90@gmail.com>
|
While building a follow-up fix on top of this branch I found a lock order inversion that can deadlock the whole VM: the spilled list walkers take the VM wide vgic_spilled_lock and then the candidate interrupt lock, while the emulation paths hold the interrupt lock and take the spilled lock inside vgic_add_lr and vgic_add_spilled. One core in vgic_refill_lrs holding the spilled lock and spinning on an interrupt lock, and another core writing a register of that same spilled interrupt holding its lock and spinning on the spilled lock, wait on each other forever. The inverted order in vgic_refill_lrs actually predates this branch, but the LRENP commit in this series extended it to vgic_eoir_highest_spilled_active, so I fixed both here: 430410b restructures the walkers to pick the candidate under the spilled lock, release it, and revalidate under both locks taken in the canonical order. |
The spilled list walkers took the VM wide vgic_spilled_lock and then the candidate interrupt's lock, while every other path holds the interrupt lock and takes the spilled lock inside it (vgic_add_lr, vgic_add_spilled). Two cpus can deadlock: one holds the spilled lock in vgic_refill_lrs spinning on an interrupt lock, while the other holds that interrupt lock in vgic_int_set_field spinning on the spilled lock in vgic_add_lr. Restructure vgic_refill_lrs and vgic_eoir_highest_spilled_active to pick the candidate under the spilled lock, release it, take the interrupt lock, retake the spilled lock, and revalidate that the candidate is still spilled and eligible before acting, rescanning otherwise. The inverted order in vgic_refill_lrs predates this branch; vgic_eoir_highest_spilled_active gained the same pattern in an earlier commit of this series. Signed-off-by: Jose Martins <josemartins90@gmail.com>
4 participants
This PR introduces a set of fixes related to issues with vGIC List Register management. Among others, it addresses issues #162.