Re: [PATCH v5 20/49] KVM: arm64: gic-v5: Add GICv5 IRS IODEV and MMIO emulation

[Sascha Bischoff <Sascha.Bischoff@arm.com>]

On Fri, 2026-08-07 at 14:34 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> - [High] 64-bit IRS MMIO registers lack support for 32-bit accesses,
> leading to unhandled MMIO aborts and broken read-modify-write
> behavior.
> - [Medium] Missing locking in the IRS IST MMIO read handler allows
> guests to observe logically torn register states.
> - [Medium] Lockless, sequential updates to GICV5_IRS_CR1 can result
> in a torn MMIO write.
> --
> 
> --- Patch [20]: [PATCH v5 20/49] KVM: arm64: gic-v5: Add GICv5 IRS
> IODEV and MMIO emulation ---
> commit e3d747ac0ec8b60b894092ad4f2dc0c7959423ef
> Author: Sascha Bischoff <Sascha.Bischoff@arm.com>
> 
> KVM: arm64: gic-v5: Add GICv5 IRS IODEV and MMIO emulation
>     
> In order to properly support GICv5-based VMs in KVM, emulate the
> CONFIG_FRAME for a virtual IRS. This emulation needs to handle guest
> accesses to the MMIO region and mimic the behaviour of a real IRS.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-irs-v5.c
> > b/arch/arm64/kvm/vgic/vgic-irs-v5.c
> > new file mode 100644
> > index 0000000000000..c6280795ab551
> > --- /dev/null
> > +++ b/arch/arm64/kvm/vgic/vgic-irs-v5.c
> 
> [ ... ]
> 
> > @@ -0,0 +1,765 @@
> [ ... ]
> > +static void vgic_v5_mmio_write_irs_misc(struct kvm_vcpu *vcpu,
> > gpa_t addr,
> > +					unsigned int len, unsigned
> > long val)
> > +{
> > +	struct vgic_v5_irs *irs = vgic_v5_get_irs(vcpu);
> > +	struct vgic_dist *vgic = vgic_v5_get_vgic(vcpu);
> > +	const size_t offset = addr & (SZ_64K - 1);
> > +
> > +	switch (offset) {
> [ ... ]
> > +	case GICV5_IRS_CR1:
> > +		irs->cr1.sh = FIELD_GET(GICV5_IRS_CR1_SH, val);
> > +		irs->cr1.oc = FIELD_GET(GICV5_IRS_CR1_OC, val);
> > +		irs->cr1.ic = FIELD_GET(GICV5_IRS_CR1_IC, val);
> > +		irs->cr1.ist_ra = !!(val & GICV5_IRS_CR1_IST_RA);
> > +		irs->cr1.ist_wa = !!(val & GICV5_IRS_CR1_IST_WA);
> > +		irs->cr1.vmt_ra = !!(val & GICV5_IRS_CR1_VMT_RA);
> > +		irs->cr1.vpet_ra = !!(val &
> > GICV5_IRS_CR1_VPET_RA);
> > +		irs->cr1.vmd_ra = !!(val & GICV5_IRS_CR1_VMD_RA);
> > +		irs->cr1.vmd_wa = !!(val & GICV5_IRS_CR1_VMD_WA);
> > +		irs->cr1.vped_ra = !!(val &
> > GICV5_IRS_CR1_VPED_RA);
> > +		irs->cr1.vped_wa = !!(val &
> > GICV5_IRS_CR1_VPED_WA);
> > +		return;
> 
> [Severity: Medium]
> Since these sequential writes are performed without holding
> config_lock,
> is there a risk of data races? If multiple vCPUs concurrently write
> different values to GICV5_IRS_CR1, it seems like the internal KVM
> state
> for irs->cr1 could end up in a torn state combining the concurrent
> writes.

While I agree that this would break, it would be broken software, and
we don't actually use the values written to this register - we just
provide them back to the guest if and what it reads them. Therefore, I
don't think that this is worth the extra complications of changing it.

> 
> [ ... ]
> 
> > +static unsigned long vgic_v5_mmio_read_irs_ist(struct kvm_vcpu
> > *vcpu,
> > +					       gpa_t addr,
> > unsigned int len)
> > +{
> > +	struct vgic_v5_irs *irs = vgic_v5_get_irs(vcpu);
> > +	const size_t offset = addr & (SZ_64K - 1);
> > +	u64 value = 0;
> > +
> > +	switch (offset) {
> [ ... ]
> > +	case GICV5_IRS_IST_BASER:
> > +		value = FIELD_PREP(GICV5_IRS_IST_BASER_ADDR_MASK,
> > +				   irs->ist_baser.addr >>
> > GICV5_IRS_IST_BASER_ADDR_SHIFT);
> > +		if (irs->ist_baser.valid)
> > +			value |= GICV5_IRS_IST_BASER_VALID;
> > +		break;
> 
> [Severity: Medium]
> Can guests observe logically torn register states here? The write
> handler
> vgic_v5_mmio_write_irs_ist() correctly holds config_lock to
> sequentially
> update irs->ist_baser.valid and irs->ist_baser.addr, but this read
> handler accesses them locklessly. If a vCPU reads GICV5_IRS_IST_BASER
> concurrently with a write, it could see an impossible state combining
> the
> old address with the new valid bit or vice-versa.

As above, this is something that we shouldn't need to guard against. If
software both writes and reads this register at the same time from
multiple vCPUs, then that software should expect races. Frankly, this
is broken software, and it will potentially get gibberish back.

> 
> [ ... ]
> 
> > +static void vgic_v5_mmio_write_irs_ist(struct kvm_vcpu *vcpu,
> > gpa_t addr,
> > +				       unsigned int len, unsigned
> > long val)
> > +{
> > +	struct vgic_v5_irs *irs = vgic_v5_get_irs(vcpu);
> > +	const size_t offset = addr & (SZ_64K - 1);
> > +	enum gicv5_vcpu_cmd cmd = LPI_VIST_MAKE_INVALID;
> > +
> > +	switch (offset) {
> [ ... ]
> > +	case GICV5_IRS_IST_BASER: {
> 
> [Severity: High]
> Does this strict matching on exact offsets break read-modify-write
> for
> 32-bit accesses to the high half of the register? If a guest performs
> a
> 32-bit write to offset + 4, it looks like this switch statement drops
> it.
> Also, interpreting a 32-bit payload as a full 64-bit value without
> adjusting for the offset would corrupt the register state.

I've extended the accessors to cope with 32-bit accesses for the
IRS_IST_BASER. This is the only implemented register we have that
supports both 32- and 64-bit accesses. The rest are RAZ/WI in the
current implementation. I've also enabled the 32-bit accesses for
those, but they have no effects otherwise.

> 
> [ ... ]
> 
> > +static const struct vgic_register_region vgic_v5_irs_registers[] =
> > {
> [ ... ]
> > +	REGISTER_DESC_WITH_LENGTH(GICV5_IRS_IST_BASER,
> > vgic_v5_mmio_read_irs_ist,
> > +				  vgic_v5_mmio_write_irs_ist, 8,
> > +				  VGIC_ACCESS_64bit),
> 
> [Severity: High]
> Could omitting VGIC_ACCESS_32bit for this 64-bit register cause the
> KVM
> IO bus to reject valid 32-bit guest accesses? If KVM's mmio
> dispatcher
> rejects a 32-bit access, the request will fall back to userspace IO.
> Since
> userspace does not emulate the in-kernel IRS, this usually results in
> an
> unhandled data abort injected into the guest.
> 

See my comment above - I have added 32-bit accesses for the
IRS_IST_BASER.

Thanks,
Sascha