Re: [PATCH v4 42/48] KVM: selftests: Add VGICv5 IRS_REGS attribute tests

On Fri, 2026-07-24 at 12:06 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> - [Medium] Using a `u64` variable for 32-bit KVM device attributes
> causes test logic failures on Big-Endian architectures.
> - [Medium] `IST_BASER` is configured to point to the VGIC MMIO region
> instead of guest RAM.
> - [Medium] Spurious test failure on hardware supporting 8 priority
> bits.
> --
> 
> --- Patch [42]: [PATCH v4 42/48] KVM: selftests: Add VGICv5 IRS_REGS
> attribute tests ---
> commit 7457ae5ec0d03e64cfdb7419437558dc7acaf9df
> Author: Sascha Bischoff <[email protected]>
> Subject: KVM: selftests: Add VGICv5 IRS_REGS attribute tests
> 
> Add coverage for the VGICv5 IRS_REGS attribute group. Migration needs
> a
> userspace-visible view of the virtual IRS register state, but KVM
> must
> only expose valid registers and must reject state that cannot be
> restored
> safely.
> 
> > diff --git a/tools/testing/selftests/kvm/arm64/vgic_v5.c
> > b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> > --- a/tools/testing/selftests/kvm/arm64/vgic_v5.c
> > +++ b/tools/testing/selftests/kvm/arm64/vgic_v5.c
> 
> [ ... ]
> 
> > +static void test_vgic_v5_irs_regs_attrs(void)
> > +{
> 
> [ ... ]
> 
> > +	ret = __kvm_device_attr_get(v.gic_fd,
> > KVM_DEV_ARM_VGIC_GRP_IRS_REGS,
> > +				    GICV5_IRS_IDR1, &val);
> > +	TEST_ASSERT(!ret, "GICv5 IRS_REGS get IDR1 failed");
> > +	val |= FIELD_PREP(GICV5_IRS_IDR1_PRIORITY_BITS, 0x7);
> > +	ret = __kvm_device_attr_set(v.gic_fd,
> > KVM_DEV_ARM_VGIC_GRP_IRS_REGS,
> > +				    GICV5_IRS_IDR1, &val);
> > +	TEST_ASSERT(ret && errno == EINVAL, "GICv5 IRS_REGS
> > accepted bad IDR1");
> 
> [Severity: Medium]
> Will this assertion falsely fail on hardware that supports 8 priority
> bits?
> 
> The architecture allows up to 8 priority bits, which encodes as 0x7.

It does? That's news to me!

The spec says 5, encoded as 0b100. We're explicitly trying to test
something invalid here.

> If the
> test runs on a host whose hardware legitimately supports this
> configuration,
> KVM will likely accept the write instead of returning -EINVAL,
> causing a
> spurious test failure.

And if such a host exists, then things should fail because it doesn't
match the spec.

> 
> [ ... ]
> 
> > +	ret = __kvm_device_attr_get(v.gic_fd,
> > KVM_DEV_ARM_VGIC_GRP_IRS_REGS,
> > +				    GICV5_IRS_IDR5, &val);
> > +	TEST_ASSERT(!ret, "GICv5 IRS_REGS get IDR5 failed");
> > +	val++;
> > +	ret = __kvm_device_attr_set(v.gic_fd,
> > KVM_DEV_ARM_VGIC_GRP_IRS_REGS,
> > +				    GICV5_IRS_IDR5, &val);
> > +	TEST_ASSERT(ret && errno == EINVAL, "GICv5 IRS_REGS
> > accepted bad IDR5");
> 
> [Severity: Medium]
> Could this cause spurious test failures on Big-Endian architectures?
> 
> KVM reads and writes 32-bit attributes to the upper 32 bits of a u64
> buffer
> on BE systems. The val++ operation increments the lower 32 bits,
> leaving
> the actual 32-bit attribute value in the upper 32 bits untouched.
> When
> this is written back, KVM might legitimately accept the unchanged
> value,
> causing this assertion to fail.

This is a u64. We are incrementing a u64 and passing that back to KVM.
The value will still be incremented correctly. This has nothing to do
with endianness.

> 
> [ ... ]
> 
> > +	/* Status registers read as idle through userspace
> > accessors. */
> > +	ret = __kvm_device_attr_get(v.gic_fd,
> > KVM_DEV_ARM_VGIC_GRP_IRS_REGS,
> > +				    GICV5_IRS_SYNC_STATUSR, &val);
> > +	TEST_ASSERT(!ret && val == GICV5_IRS_SYNC_STATUSR_IDLE,
> > +		    "GICv5 IRS_REGS SYNC_STATUSR is not idle");
> 
> [ ... ]
> 
> > +	/* Userspace restore of IST_BASER updates state without
> > allocating ISTs. */
> > +	val = FIELD_PREP(GICV5_IRS_IST_BASER_ADDR_MASK,
> > +			 GICV5_IRS_CONFIG_BASE_GPA >>
> > GICV5_IRS_IST_BASER_ADDR_SHIFT);
> > +	val |= GICV5_IRS_IST_BASER_VALID;
> > +	ret = __kvm_device_attr_set(v.gic_fd,
> > KVM_DEV_ARM_VGIC_GRP_IRS_REGS,
> > +				    GICV5_IRS_IST_BASER, &val);
> > +	TEST_ASSERT(!ret, "GICv5 IRS_REGS failed to restore
> > IST_BASER");
> 
> [Severity: Medium]
> Is it intentional to configure the Interrupt Status Table to overlap
> with the
> VGIC's own MMIO region here?

Ish. We just need an aligned address that we can write to the
IRS_IST_BASER and read back again. It is never used.

I've changed this to use a clear placeholder here.

> 
> GICV5_IRS_CONFIG_BASE_GPA is the base address of the IRS MMIO region,
> but
> the IST should typically reside in standard guest RAM (such as
> VGIC_V5_LPI_IST_BASE_GPA). If KVM adds validation in the future to
> ensure
> IST_BASER points to a valid memory slot instead of an MMIO region,
> could
> this configuration be rejected and break the test?
> 
> [ ... ]
> 
Thanks,
Sascha