Re: [PATCH v1 3/4] KVM: arm64: Convert KVM_ARM_VCPU_* features into an enum

[Marc Zyngier <maz@kernel.org>]

On Mon, 14 Oct 2024 17:58:08 +0100,
Fuad Tabba <tabba@google.com> wrote:
> 
> Instead of using compile time defines, convert the KVM_ARM_VCPU_*
> features into an enum. Among other things, this reduces the
> chances of missing to update KVM_ARM_VCPU_MAX_FEATURES.
> 
> Also rename KVM_VCPU_MAX_FEATURES and KVM_ARM_VCPU_VALID_FEATURES
> to KVM_ARM_VCPU_MAX_FEATURES and KVM_ARM_VCPU_VALID_FEATURES in
> order to match the features they're counting.
> 
> No functional change intended.
> 
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  2 +-
>  arch/arm64/include/uapi/asm/kvm.h | 25 ++++++++++++++-----------
>  arch/arm64/kvm/arm.c              | 10 +++++-----
>  3 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 85901afeb332..9b7bf4ba07a3 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -326,7 +326,7 @@ struct kvm_arch {
>  	unsigned long flags;
>  
>  	/* VM-wide vCPU feature set */
> -	DECLARE_BITMAP(vcpu_features, KVM_VCPU_MAX_FEATURES);
> +	DECLARE_BITMAP(vcpu_features, KVM_ARM_VCPU_MAX_FEATURES);
>  
>  	/* MPIDR to vcpu index mapping, optional */
>  	struct kvm_mpidr_data *mpidr_data;
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 2d5fd0ed7dff..b6ebd79b8373 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -100,17 +100,20 @@ struct kvm_regs {
>  #define KVM_VGIC_V3_REDIST_SIZE		(2 * SZ_64K)
>  #define KVM_VGIC_V3_ITS_SIZE		(2 * SZ_64K)
>  
> -#define KVM_ARM_VCPU_POWER_OFF		0 /* CPU is started in OFF state */
> -#define KVM_ARM_VCPU_EL1_32BIT		1 /* CPU running a 32bit VM */
> -#define KVM_ARM_VCPU_PSCI_0_2		2 /* CPU uses PSCI v0.2 */
> -#define KVM_ARM_VCPU_PMU_V3		3 /* Support guest PMUv3 */
> -#define KVM_ARM_VCPU_SVE		4 /* enable SVE for this CPU */
> -#define KVM_ARM_VCPU_PTRAUTH_ADDRESS	5 /* VCPU uses address authentication */
> -#define KVM_ARM_VCPU_PTRAUTH_GENERIC	6 /* VCPU uses generic authentication */
> -#define KVM_ARM_VCPU_HAS_EL2		7 /* Support nested virtualization */
> -
> -#define KVM_VCPU_MAX_FEATURES 8
> -#define KVM_VCPU_VALID_FEATURES	(BIT(KVM_VCPU_MAX_FEATURES) - 1)
> +enum kvm_arm_vcpu_features {
> +	KVM_ARM_VCPU_POWER_OFF = 0,	/* CPU is started in OFF state */
> +	KVM_ARM_VCPU_EL1_32BIT,		/* CPU running a 32bit VM */
> +	KVM_ARM_VCPU_PSCI_0_2,		/* CPU uses PSCI v0.2 */
> +	KVM_ARM_VCPU_PMU_V3,		/* Support guest PMUv3 */
> +	KVM_ARM_VCPU_SVE,		/* enable SVE for this CPU */
> +	KVM_ARM_VCPU_PTRAUTH_ADDRESS,	/* VCPU uses address authentication */
> +	KVM_ARM_VCPU_PTRAUTH_GENERIC,	/* VCPU uses generic authentication */
> +	KVM_ARM_VCPU_HAS_EL2,		/* Support nested virtualization */
> +
> +	KVM_ARM_VCPU_MAX_FEATURES,	/* Must be last */
> +};

This is going to break any userspace that has code shaped like this:

#ifdef KVM_ARM_VCPU_SVE
[...]
#endif

Not necessarily a good pattern, but nonetheless a valid one. So you
*must* preserve the #defines, even if you define them along the lines
of:

#define	KVM_ARM_VCPU_SVE	KVM_ARM_VCPU_SVE

Thanks,

	M.

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