Re: [PATCH v4 46/48] KVM: selftests: Add VGICv5 SPI injection tests

On Fri, 2026-07-24 at 12:15 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [Medium] Writes to GICv5 control-plane system registers are missing
> the mandatory immediate isb() synchronization.
> --
> 
> --- Patch [46]: [PATCH v4 46/48] KVM: selftests: Add VGICv5 SPI
> injection tests ---
> commit 194658b92777c0e76e825f04069b3e506ee164f3
> Author: Sascha Bischoff <[email protected]>
> 
> KVM: selftests: Add VGICv5 SPI injection tests
> 
> Add VGICv5 SPI delivery coverage through KVM_IRQ_LINE. SPIs are
> configured through the virtual IRS and delivered through the GICv5
> CPU
> interface, so the test needs to exercise both the userspace injection
> path and the guest-side IRS programming path.
> 
> > diff --git a/tools/testing/selftests/kvm/arm64/vgic_v5.c
> > b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> > index 22ae726b17be8..cec994a77c993 100644
> > --- a/tools/testing/selftests/kvm/arm64/vgic_v5.c
> > +++ b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> [ ... ]
> > @@ -41,7 +44,71 @@ static struct kvm_vgic_v5_ist
> > vgic_v5_ist_attr(void *spi_ist, size_t spi_size,
> >  	};
> >  }
> >  
> > +static void gicv5_setup_and_enable_hwirq(u64 hwirq, u32
> > target_vpe)
> > +{
> > +	u64 val;
> > +
> > +	val = hwirq | FIELD_PREP(GICV5_GIC_CDPRI_PRIORITY_MASK,
> > +				 GICV5_IRQ_DEFAULT_PRI);
> > +	gic_insn(val, CDPRI);
> > +
> > +	val = hwirq | FIELD_PREP(GICV5_GIC_CDAFF_IAFFID_MASK,
> > target_vpe);
> > +	gic_insn(val, CDAFF);
> > +
> > +	gic_insn(hwirq, CDEN);
> 
> [Severity: Medium]
> Are isb() barriers required after these control-plane system register
> writes?
> 

No.

> Unless these specific GICv5 CD registers are architecturally defined
> as
> self-synchronizing, ARM64 requires an immediate isb() after every
> control-plane system register write to ensure the CPU pipeline
> observes
> the new configuration state.

These are all operating on the same interrupt to change the state of
it. This does not need explicit synchronisation.

> 
> > +}
> [ ... ]
> > +static void guest_spi_irq_handler(struct ex_regs *regs)
> > +{
> > +	bool valid;
> > +	u32 hwirq;
> > +	u64 ia;
> > +
> > +	ia = gicr_insn(CDIA);
> > +	valid = GICV5_GICR_CDIA_VALID(ia);
> > +
> > +	if (!valid)
> > +		return;
> > +
> > +	gsb_ack();
> > +	isb();
> > +
> > +	hwirq = FIELD_GET(GICV5_GICR_CDIA_INTID, ia);
> > +
> > +	GUEST_ASSERT_EQ(hwirq,
> > gicv5_spi_hwirq(READ_ONCE(spi_line_expected)));
> > +
> > +	gic_insn(hwirq, CDDI);
> > +	gic_insn(0, CDEOI);
> 
> [Severity: Medium]
> Should these system register writes also be followed by isb()
> barriers?
> 
> Similar to the above, without explicit synchronization, the pipeline
> might not immediately observe the updated state before the exception
> handler completes.

The part that needs synchronisation is the CDIA, and we have that in
the form of a gsb_ack() and isb(). The rest does not.

> 
> > +
> > +	GUEST_DONE();
> > +}
>