Re: [PATCH v4 13/48] KVM: arm64: gic-v5: Implement VPE IRS MMIO Ops

On Fri, 2026-07-24 at 11:21 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> - [High] Missing REQ_DB (Request Doorbell) flag when configuring the
> VPE doorbell, contradicting the explicit intent to force doorbell
> generation.
> - [High] Missing VPE engine idle check at the beginning of
> `vgic_v5_irs_set_up_vpe`, risking command corruption if the engine is
> busy.
> --
> 
> --- Patch [13]: [PATCH v4 13/48] KVM: arm64: gic-v5: Implement VPE
> IRS MMIO Ops ---
> commit 92182082105efc7200f554f8b53732d80e3ea711
> Author: Sascha Bischoff <[email protected]>
> 
> KVM: arm64: gic-v5: Implement VPE IRS MMIO Ops
> 
> This commit introduces interfaces to validate and configure VPEs via
> the
> host's IRS. It extends the existing doorbell-based commands by adding
> VPE_MAKE_VALID.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-v5.c
> > b/arch/arm64/kvm/vgic/vgic-v5.c
> > index 21252bd11d96f..826c2284a1453 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v5.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> 
> [ ... ]
> 
> > @@ -333,10 +345,73 @@ static int vgic_v5_irs_set_vist_invalid(u16
> > vm_id, bool spi_ist)
> >  	return __vgic_v5_irs_update_vist_validity(vm_id, spi_ist,
> > true);
> >  }
> >  
> > +static int vgic_v5_irs_set_up_vpe(u16 vm_id, u16 vpe_id,
> > +				  irq_hw_number_t db_hwirq)
> > +{
> > +	u64 vmap_vper, dbr, selr;
> > +	u32 statusr, cr0;
> > +	int ret;
> > +
> > +	guard(raw_spinlock_irqsave)(&global_irs_lock);
> > +
> > +	/* Make sure that we are idle to begin with */
> > +	ret = vgic_v5_irs_wait_for_vm_op();
> > +	if (ret)
> > +		return ret;
> 
> [Severity: High]
> Does this code also need to verify that the VPE engine is idle?

No. If it is busy when we get here than things have already gone wrong
elsewhere. We'll try and do something, and likely will remain busy
until we time out and return an error. Again.

> 
> If a previous operation timed out and returned an error, the function
> could exit early and leave the hardware engine busy. When called
> again
> for the next VPE, the VM engine check above would pass, but writing
> to
> GICV5_IRS_VPE_SELR below could corrupt the command if the VPE engine
> is
> still busy.

If a previous operation timed out, then that's likely a hardware bug
that caused it. Things would already be rather broken.

> 
> > +
> > +	/* Mark the VPE as valid */
> > +	vmap_vper = FIELD_PREP(GICV5_IRS_VMAP_VPER_VPE_ID, vpe_id)
> > |
> > +		    FIELD_PREP(GICV5_IRS_VMAP_VPER_VM_ID, vm_id) |
> > +		    GICV5_IRS_VMAP_VPER_M;
> > +	irs_writeq_relaxed(vmap_vper, GICV5_IRS_VMAP_VPER);
> 
> [ ... ]
> 
> > +	/*
> > +	 * The VPE has not yet run. Therefore, make sure that all
> > interrupts
> > +	 * will generate a doorbell.
> > +	 */
> > +	dbr = FIELD_PREP(GICV5_IRS_VPE_DBR_INTID, db_hwirq) |
> > +	      GICV5_IRS_VPE_DBR_DBV;
> > +	irs_writeq_relaxed(dbr, GICV5_IRS_VPE_DBR);
> 
> [Severity: High]
> Is the GICV5_IRS_VPE_DBR_REQ_DB flag missing here?
> 
> The comment explicitly states the intent is to ensure all interrupts
> will
> generate a doorbell. Without the request doorbell flag, could the GIC
> hardware
> fail to generate doorbells for interrupts that become pending before
> the VPE
> first runs?
> 

No. The request doorbell flag is there to request a doorbell, NOT to
configure it. In normal operation, doorbells are requested via the
ICH_CONTEXTR_EL2 when making a VPE non-resident (and when said VPE is
entering WFI).

Thanks,
Sascha