Re: [PATCH v4 32/48] KVM: arm64: gic-v5: Handle userspace accesses to IRS MMIO region

On Fri, 2026-07-24 at 12:05 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> - [High] Userspace writes to `GICV5_IRS_SPI_CFGR` are silently
> ignored without validation, breaking restoration of SPI
> configurations across live migrations.
> - [Medium] The 64-bit registers GICV5_IRS_IST_BASER and
> GICV5_IRS_SPI_VMR lack support for 32-bit guest MMIO accesses,
> violating the GIC architecture.
> - [Medium] KVM_HAS_DEVICE_ATTR for KVM_DEV_ARM_VGIC_GRP_IRS_REGS
> incorrectly fails with -EINVAL if VCPU 0 is not yet created, breaking
> feature enumeration.
> --
> 
> --- Patch [32]: [PATCH v4 32/48] KVM: arm64: gic-v5: Handle userspace
> accesses to IRS MMIO region ---
> commit 9b79670b7941a4c70c01c7a66f84cbfecafe7755
> Author: Sascha Bischoff <[email protected]>
> 
> KVM: arm64: gic-v5: Handle userspace accesses to IRS MMIO region
> 
> As part of saving and restoring state of a GICv5-based system,
> userspace is
> required to save/restore the IRS MMIO registers. Provide accessors to
> read
> and write the IRS MMIO state. Where possible, the existing access
> mechanisms
> are used, but for some registers the access is handled a bit
> differently as
> they have wider effects.
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-irs-v5.c
> > b/arch/arm64/kvm/vgic/vgic-irs-v5.c
> > --- a/arch/arm64/kvm/vgic/vgic-irs-v5.c
> > +++ b/arch/arm64/kvm/vgic/vgic-irs-v5.c
> 
> [ ... ]
> 
> > @@ -455,84 +584,217 @@ static void
> > vgic_v5_mmio_write_irs_ist(struct kvm_vcpu *vcpu, gpa_t addr,
> >  	case GICV5_IRS_CR0:
> >  		vgic->enabled = !!(val & GICV5_IRS_CR0_IRSEN);
> >  		break;
> > +	case GICV5_IRS_SPI_CFGR:
> > +		break;
> 
> [Severity: High]
> Does this silently ignore userspace writes to restore SPI
> configurations?

Yes. Explicitly.

> 
> The commit message mentions blocking userspace from setting anything
> that
> doesn't match what has been set already. However, the uaccess write
> handler
> here just executes a break and returns 0 without validating the write
> or
> applying any updates.
> 
> Could this cause SPI configurations (like edge vs level triggered) to
> be lost
> during VM migration, permanently leaving restored SPIs in their
> default state?

No, those are stored in the IST. As part of restoring the IST (one of
the next commits) those can be and are restored.

> 
> > +	case GICV5_IRS_IIDR:
> > +		fallthrough;
> > +	case GICV5_IRS_AIDR:
> > +		break;
> >  	default:
> > -		return;
> > +		return -EINVAL;
> >  	}
> > +
> > +	return 0;
> >  }
> 
> [ ... ]
> 
> > @@ -494,35 +673,50 @@ 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),
> > +	REGISTER_DESC_WITH_LENGTH_UACCESS(GICV5_IRS_IST_BASER,
> > +					 
> > vgic_v5_mmio_read_irs_ist,
> > +					 
> > vgic_v5_mmio_write_irs_ist, NULL,
> > +					 
> > vgic_v5_mmio_uaccess_write_irs, 8,
> > +					  VGIC_ACCESS_64bit),
> 
> [Severity: Medium]
> Should these 64-bit registers support 32-bit MMIO accesses?
> 
> The region descriptor specifies only VGIC_ACCESS_64bit. The GIC
> architecture
> mandates that 64-bit registers must also support 32-bit accesses, so
> valid
> 32-bit guest reads or writes might be improperly rejected.
> 
> Additionally, if 32-bit accesses were permitted, would the write
> handler in
> vgic_v5_mmio_write_irs_ist() improperly clobber the upper 32 bits? It
> appears to lack the update_64bit_reg() wrapper used elsewhere in KVM
> to
> handle partial writes safely.

Right now,none of the IRS MMIO registers support 32-bit accesses. This
is a potential issue that I will look into in the future.

> 
> [ ... ]
> 
> > diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> 
> [ ... ]
> 
> > @@ -786,6 +786,9 @@ int vgic_v5_parse_attr(struct kvm_device *dev,
> > struct kvm_device_attr *attr,
> >  		mpidr_reg = VGIC_TO_MPIDR(vgic_mpidr);
> >  		reg_attr->vcpu = kvm_mpidr_to_vcpu(dev->kvm,
> > mpidr_reg);
> >  		break;
> > +	case KVM_DEV_ARM_VGIC_GRP_IRS_REGS:
> > +		reg_attr->vcpu = kvm_get_vcpu(dev->kvm, 0);
> > +		break;
> >  	default:
> >  		return -EINVAL;
> >  	}
> 
> [Severity: Medium]
> Does this break KVM_HAS_DEVICE_ATTR feature enumeration before VCPUs
> are
> created?
> 
> If a VMM queries KVM_HAS_DEVICE_ATTR for
> KVM_DEV_ARM_VGIC_GRP_IRS_REGS
> immediately after creating the VGIC device, kvm_get_vcpu(dev->kvm, 0)
> will
> return NULL.

We mandate that the GIC is initialised before interacting with it. Just
creating it isn't sufficient. In order to init the GIC, the vCPUs also
need to be present.

> 
> Because vgic_v5_parse_attr() returns -EINVAL when reg_attr->vcpu is
> NULL,
> this causes feature probing to yield false negatives. As IRS
> registers are
> VM-wide features, can we avoid this dependency on VCPU 0 during
> enumeration?
> 
Thanks,
Sascha