Re: [PATCH v3 3/3] KVM: arm64: selftests: Check ID regs are immutable after a failed run

[Lorenzo Stoakes (ARM)]

On Tue, Sep 01, 2026 at 07:18:50PM +0100, Mark Brown wrote:
> From: Fuad Tabba <fuad.tabba@xxxxxxxxx>
>
> Add a set_id_regs case covering ID register immutability when a vCPU's
> first KVM_RUN fails after finalization but before
> KVM_ARCH_FLAG_HAS_RAN_ONCE is set. The test provokes such a failure with
> a PMUv3-enabled vCPU whose PMU is left uninitialized, then checks that
> KVM_SET_ONE_REG on the feature and implementation ID registers, and
> KVM_CREATE_DEVICE for a vGIC, are all rejected with -EBUSY.
>
> Assisted-by: Antigravity:gemini-3.1-pro

NIT: we changed the rules for the Assisted-by tag, it only needs to be
'Assisted-by: LLM' now. Not a huge big deal but FYI :>)

https://docs.kernel.org/process/coding-assistants.html

> Signed-off-by: Fuad Tabba <fuad.tabba@xxxxxxxxx>
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
> Link: https://patch.msgid.link/20260805064740.3013538-1-fuad.tabba@xxxxxxxxx

LGTM so:

Acked-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>

> ---
>  tools/testing/selftests/kvm/arm64/set_id_regs.c | 106 +++++++++++++++++++++++-
>  1 file changed, 105 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kvm/arm64/set_id_regs.c b/tools/testing/selftests/kvm/arm64/set_id_regs.c
> index 7429a1055df5..10849d21c0dd 100644
> --- a/tools/testing/selftests/kvm/arm64/set_id_regs.c
> +++ b/tools/testing/selftests/kvm/arm64/set_id_regs.c
> @@ -13,6 +13,7 @@
>  #include "kvm_util.h"
>  #include "processor.h"
>  #include "test_util.h"
> +#include "vgic.h"
>  #include <linux/bitfield.h>
>
>  enum ftr_type {
> @@ -803,6 +804,107 @@ static void test_reset_preserves_id_regs(struct kvm_vcpu *vcpu)
>  	ksft_test_result_pass("%s\n", __func__);
>  }
>
> +/*
> + * ID registers must stay immutable even when a vCPU's first KVM_RUN fails
> + * after finalization but before KVM_ARCH_FLAG_HAS_RAN_ONCE is set.
> + */
> +static void test_idreg_frozen_after_failed_run(void)
> +{
> +	static const u32 imp_id_regs[] = {
> +		SYS_MIDR_EL1,
> +		SYS_REVIDR_EL1,
> +		SYS_AIDR_EL1,
> +	};
> +	struct kvm_vcpu_init init;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +	int r;
> +
> +	if (!kvm_has_cap(KVM_CAP_ARM_PMU_V3)) {
> +		ksft_print_msg("PMUv3 unsupported, cannot fail the first run\n");
> +		ksft_test_result_skip("%s\n", __func__);
> +		return;
> +	}
> +
> +	/* Skip the default vGIC so the KVM_CREATE_DEVICE gate is reachable. */
> +	test_disable_default_vgic();
> +
> +	vm = vm_create(1);
> +	vm_enable_cap(vm, KVM_CAP_ARM_WRITABLE_IMP_ID_REGS, 0);
> +	kvm_get_default_vcpu_target(vm, &init);
> +	init.features[0] |= (1 << KVM_ARM_VCPU_PMU_V3);
> +	vcpu = aarch64_vcpu_add(vm, 0, &init, guest_code);
> +	kvm_arch_vm_finalize_vcpus(vm);
> +
> +	/*
> +	 * A PMUv3 vCPU left without PMU init is rejected by
> +	 * kvm_arm_pmu_v3_enable(), which runs after sysreg finalization.
> +	 */
> +	r = _vcpu_run(vcpu);
> +	TEST_ASSERT(r < 0 && errno == EINVAL,
> +		    "first KVM_RUN should fail post-finalization: r=%d errno=%d",
> +		    r, errno);

Ah nice determinate way of triggering the issue.

> +
> +	/*
> +	 * Feature ID registers: use values that would have been accepted before
> +	 * finalization, so that a rejection means the registers are final
> +	 * rather than the value being invalid.
> +	 */
> +	for (int i = 0; i < ARRAY_SIZE(test_regs); i++) {
> +		const struct reg_ftr_bits *ftr_bits = test_regs[i].ftr_bits;
> +		u64 reg = KVM_ARM64_SYS_REG(test_regs[i].reg);
> +		u64 val = vcpu_get_reg(vcpu, reg);
> +
> +		for (int j = 0; ftr_bits[j].type != FTR_END; j++) {
> +			u64 ftr = (val & ftr_bits[j].mask) >> ftr_bits[j].shift;
> +			u64 safe = get_safe_value(&ftr_bits[j], ftr);
> +			u64 new_val;
> +
> +			if (safe == ftr)
> +				continue;
> +
> +			new_val = (val & ~ftr_bits[j].mask) |
> +				  (safe << ftr_bits[j].shift);
> +
> +			r = __vcpu_set_reg(vcpu, reg, new_val);
> +			TEST_ASSERT(r < 0 && errno == EBUSY,
> +				    "%s write after failed first run: r=%d errno=%d",
> +				    ftr_bits[j].name, r, errno);
> +			TEST_ASSERT_EQ(vcpu_get_reg(vcpu, reg), val);
> +		}
> +
> +		/* A write matching the finalized value is still accepted. */
> +		vcpu_set_reg(vcpu, reg, val);
> +	}
> +
> +	/*
> +	 * The VM-wide implementation ID registers are gated separately. Bit 0
> +	 * is within the writable mask of all three, so flipping it is a change
> +	 * KVM would otherwise accept.
> +	 */

Nice.

> +	for (int i = 0; i < ARRAY_SIZE(imp_id_regs); i++) {
> +		u64 reg = KVM_ARM64_SYS_REG(imp_id_regs[i]);
> +		u64 val = vcpu_get_reg(vcpu, reg);
> +
> +		r = __vcpu_set_reg(vcpu, reg, val ^ 1);

Ah literally a flip.

> +		TEST_ASSERT(r < 0 && errno == EBUSY,
> +			    "implementation ID reg write after failed first run: r=%d errno=%d",
> +			    r, errno);
> +		TEST_ASSERT_EQ(vcpu_get_reg(vcpu, reg), val);
> +	}
> +
> +	/* Creating an in-kernel irqchip would change the ID registers too. */
> +	if (kvm_supports_vgic_v3()) {
> +		r = __kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3);
> +		TEST_ASSERT(r < 0 && errno == EBUSY,
> +			    "vGIC creation after failed first run: r=%d errno=%d",
> +			    r, errno);

I guess in general it's making sure - where there should now be finalisation on
reg setting that this is actually enforced?

> +	}
> +
> +	kvm_vm_free(vm);
> +	ksft_test_result_pass("%s\n", __func__);
> +}
> +
>  int main(void)
>  {
>  	struct kvm_vcpu *vcpu;
> @@ -828,7 +930,7 @@ int main(void)
>
>  	ksft_print_header();
>
> -	test_cnt = 3 + MPAM_IDREG_TEST + MTE_IDREG_TEST;
> +	test_cnt = 4 + MPAM_IDREG_TEST + MTE_IDREG_TEST;
>  	for (i = 0; i < ARRAY_SIZE(test_regs); i++)
>  		for (j = 0; test_regs[i].ftr_bits[j].type != FTR_END; j++)
>  			test_cnt++;
> @@ -847,5 +949,7 @@ int main(void)
>
>  	kvm_vm_free(vm);
>
> +	test_idreg_frozen_after_failed_run();
> +
>  	ksft_finished();
>  }
>
> --
> 2.47.3
>
>

--
Cheers, Lorenzo