Re: [PATCH v1 2/4] KVM: arm64: Add compile-time type check for register in __vcpu_assign_sys_reg()

[Marc Zyngier <maz@kernel.org>]

On Mon, 27 Oct 2025 11:39:41 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> The 'r' (register) and 'val' (value) parameters in
> __vcpu_assign_sys_reg() can be easily transposed. The register ID is an
> enum, whereas the value is a u64. This has led to bugs in the past, and
> the risk was increased by the historically inconsistent parameter
> ordering of the vcpu_write_sys_reg() function.
> 
> To prevent this class of bugs, add a compile-time type compatibility check
> to prevent the 'r' parameter from having a 'u64' type.
> 
> This directly catches the erroneous transposition (passing the 'u64'
> value as 'r') while remaining flexible, as it does not force 'r' to
> be a specific enum type.

I don't think that's enough. It is too easy to pass a u32 and
accidentally bypass this check. I really think we should redefine the
register identifiers to be a fixed value structure, something like
this:

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7501a2ee4dd44..18f65dbff2379 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -624,6 +624,12 @@ enum vcpu_sysreg {
 	NR_SYS_REGS	/* Nothing after this line! */
 };
 
+struct vcpu_sys_reg {
+	enum vcpu_sysreg reg;
+};
+
+static const struct vcpu_sys_reg vcpu_sys_reg_SCTLR_EL1 = { SCTLR_EL1 };
+
 struct kvm_sysreg_masks {
 	struct {
 		u64	res0;

and then enforce that whatever is interpreted as a system register is
actually one of these structures (with some creative cpp wrapping to
make it less awkward).

In a way, this is similar to what we are doing for page table
manipulation, where pte_t is a structure wrapping a u64.

If we move to such construct, it becomes impossible to swap value and
register, and we can stop worrying about that.

Thoughts?

	M.

-- 
Without deviation from the norm, progress is not possible.