Re: [PATCH v2 07/12] KVM: arm64: Rework specifying restricted features for protected VMs

[Marc Zyngier <maz@kernel.org>]

On Fri, 22 Nov 2024 11:06:17 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> The existing code didn't properly distinguish between signed and
> unsigned features, and was difficult to read and to maintain.
> Rework it using the same method used in other parts of KVM when
> handling vcpu features.
> 
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  .../arm64/kvm/hyp/include/nvhe/fixed_config.h |   1 -
>  arch/arm64/kvm/hyp/nvhe/sys_regs.c            | 356 +++++++++---------
>  2 files changed, 186 insertions(+), 171 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/fixed_config.h b/arch/arm64/kvm/hyp/include/nvhe/fixed_config.h
> index 69e26d1a0ebe..37a6d2434e47 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/fixed_config.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/fixed_config.h
> @@ -198,7 +198,6 @@
>  	FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_APA3), ID_AA64ISAR2_EL1_APA3_PAuth) \
>  	)
>  
> -u64 pvm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id);
>  bool kvm_handle_pvm_sysreg(struct kvm_vcpu *vcpu, u64 *exit_code);
>  bool kvm_handle_pvm_restricted(struct kvm_vcpu *vcpu, u64 *exit_code);
>  void kvm_init_pvm_id_regs(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/hyp/nvhe/sys_regs.c b/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> index 3aa76b018f70..cea3b099dc56 100644
> --- a/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> +++ b/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> @@ -28,221 +28,237 @@ u64 id_aa64mmfr1_el1_sys_val;
>  u64 id_aa64mmfr2_el1_sys_val;
>  u64 id_aa64smfr0_el1_sys_val;
>  
> -/*
> - * Inject an unknown/undefined exception to an AArch64 guest while most of its
> - * sysregs are live.
> - */
> -static void inject_undef64(struct kvm_vcpu *vcpu)
> -{
> -	u64 esr = (ESR_ELx_EC_UNKNOWN << ESR_ELx_EC_SHIFT);
> -
> -	*vcpu_pc(vcpu) = read_sysreg_el2(SYS_ELR);
> -	*vcpu_cpsr(vcpu) = read_sysreg_el2(SYS_SPSR);
> -
> -	kvm_pend_exception(vcpu, EXCEPT_AA64_EL1_SYNC);
> -
> -	__kvm_adjust_pc(vcpu);
> -
> -	write_sysreg_el1(esr, SYS_ESR);
> -	write_sysreg_el1(read_sysreg_el2(SYS_ELR), SYS_ELR);
> -	write_sysreg_el2(*vcpu_pc(vcpu), SYS_ELR);
> -	write_sysreg_el2(*vcpu_cpsr(vcpu), SYS_SPSR);
> -}
> +struct pvm_feature {
> +	int shift;
> +	int width;
> +	u64 mask;
> +	u64 max_supported;
> +	bool is_signed;

Most of this is very similar to struct arm64_ftr_bits. Can you align
the types and names with it? And do you really need 'mask', which can
be derived from shift and width?

max_supported as an unsigned value is also potentially troublesome for
signed features. And does it need to be 64bit wide?

> +	bool (*vcpu_supported)(const struct kvm_vcpu *vcpu);

We really need to find a way to kill this. The two features that stick
out badly are SVE and PAuth, and I don't see why we can't eventually
track them at the VM level instead (despite the userspace API).

Thanks,

	M.

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