Re: [PATCH v4 23/48] KVM: arm64: gic-v5: Set IRICHPPIDIS based on IRS enable state

On Fri, 2026-07-24 at 11:41 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> - [High] Spurious VPE doorbells and infinite wakeup loops due to
> unpreserved IRICHPPIDIS state.
> - [Low] Missing READ_ONCE() on vgic.enabled causes a data race.
> --
> 
> --- Patch [23]: [PATCH v4 23/48] KVM: arm64: gic-v5: Set IRICHPPIDIS
> based on IRS enable state ---
> commit 40fbad6b3a2b1617d4bc4b16c3a6b176e7be5146
> Author: Sascha Bischoff <[email protected]>
> 
> KVM: arm64: gic-v5: Set IRICHPPIDIS based on IRS enable state
> 
> The GICv5 ICH_CONTEXTR_EL2 has the IRICHPPIDIS field, which allows
> the
> hypervisor to enable/disable the HPPI selection for SPIs and
> LPIs. This can be used to emulate the guest enabling/disabling the
> IRS. Therefore, make the state of this controlled by the IRS enable
> state. Thus, SPIs and LPIs can't be delivered to the guest, until it
> enables the emulated IRS, which matches the behaviour of the real
> hardware.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v5.c
> > b/arch/arm64/kvm/vgic/vgic-v5.c
> > index 4042d468431b1..7957b9da95e7c 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v5.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> > @@ -1066,6 +1066,7 @@ void vgic_v5_flush_ppi_state(struct kvm_vcpu
> > *vcpu)
> >  
> >  void vgic_v5_load(struct kvm_vcpu *vcpu)
> >  {
> > +	bool irichppidis = !vcpu->kvm->arch.vgic.enabled;
> 
> [Severity: Low]
> Is a READ_ONCE() missing here?
> 
> The vgic.enabled flag is updated concurrently by the IRS MMIO handler
> using
> WRITE_ONCE(), so reading it locklessly here might trigger KCSAN data
> race
> warnings.

OK, I agree that this could race due to this coming from emulated MMIO
accesses. I've added a READ_ONCE() here.

> 
> >  	struct vgic_v5_cpu_if *cpu_if = &vcpu-
> > >arch.vgic_cpu.vgic_v5;
> >  	u16 vm = vgic_v5_vm_id(vcpu->kvm);
> >  	u16 vpe = vgic_v5_vpe_id(vcpu);
> > @@ -1082,6 +1083,7 @@ void vgic_v5_load(struct kvm_vcpu *vcpu)
> >  	kvm_call_hyp(__vgic_v5_restore_vmcr_apr, cpu_if);
> >  
> >  	cpu_if->vgic_contextr = FIELD_PREP(ICH_CONTEXTR_EL2_V,
> > true) |
> > +				FIELD_PREP(ICH_CONTEXTR_EL2_IRICHP
> > PIDIS, irichppidis) |
> 
> [Severity: High]
> Will this IRICHPPIDIS state get lost when the VCPU enters WFI?
> 
> When the guest executes WFI, KVM calls vgic_v5_put(). It currently
> clears
> vgic_contextr before setting the doorbell bits:
> 
> void vgic_v5_put(struct kvm_vcpu *vcpu)
> {
> 	...
> 	cpu_if->vgic_contextr = 0;
> 	if (vcpu_get_flag(vcpu, IN_WFI)) {
> 		...
> 		cpu_if->vgic_contextr =
> FIELD_PREP(ICH_CONTEXTR_EL2_DB, 1) |
> 					FIELD_PREP(ICH_CONTEXTR_EL2_
> DBPM, dbpm);
> 	...
> }
> 
> If the guest disabled the IRS (so IRICHPPIDIS should be 1) and enters
> WFI
> while an SPI or LPI is pending, the hardware will see IRICHPPIDIS=0
> and DB=1.
> 
> Could this cause the hardware to inadvertently evaluate the interrupt
> and fire
> the doorbell, waking the guest?
> 
> If the guest then resumes from WFI, sees no pending interrupts, and
> executes
> WFI again, wouldn't this result in an infinite wakeup loop?

Yes, this was an oversight on my part. We should only request a
doorbell if the guest has enabled the IRS, as it otherwise would be
woken for an SPI or LPI that it cannot receive.

> 
> >  				FIELD_PREP(ICH_CONTEXTR_EL2_VPE,
> > vpe) |
> >  				FIELD_PREP(ICH_CONTEXTR_EL2_VM,
> > vm);
> > 
> 

Thanks,
Sascha