Re: [PATCH v2 05/12] KVM: arm64: Initialize feature id registers for protected VMs

[Marc Zyngier <maz@kernel.org>]

On Fri, 22 Nov 2024 11:06:15 +0000,
Fuad Tabba <tabba@google.com> wrote:
> 
> The hypervisor maintains the state of protected VMs. Initialize
> the values for feature ID registers for protected VMs, to be used
> when setting traps and when advertising features to protected
> VMs.
> 
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  .../arm64/kvm/hyp/include/nvhe/fixed_config.h |  1 +
>  arch/arm64/kvm/hyp/nvhe/pkvm.c                |  4 ++
>  arch/arm64/kvm/hyp/nvhe/sys_regs.c            | 54 +++++++++++++++++--
>  3 files changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/fixed_config.h b/arch/arm64/kvm/hyp/include/nvhe/fixed_config.h
> index d1e59b88ff66..69e26d1a0ebe 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/fixed_config.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/fixed_config.h
> @@ -201,6 +201,7 @@
>  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);
>  int kvm_check_pvm_sysreg_table(void);
>  
>  #endif /* __ARM64_KVM_FIXED_CONFIG_H__ */
> diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> index 59ff6aac514c..4ef03294b2b4 100644
> --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
> +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> @@ -381,6 +381,7 @@ static void init_pkvm_hyp_vm(struct kvm *host_kvm, struct pkvm_hyp_vm *hyp_vm,
>  	hyp_vm->kvm.created_vcpus = nr_vcpus;
>  	hyp_vm->kvm.arch.mmu.vtcr = host_mmu.arch.mmu.vtcr;
>  	hyp_vm->kvm.arch.pkvm.enabled = READ_ONCE(host_kvm->arch.pkvm.enabled);
> +	hyp_vm->kvm.arch.flags = 0;
>  	pkvm_init_features_from_host(hyp_vm, host_kvm);
>  }
>  
> @@ -419,6 +420,9 @@ static int init_pkvm_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu,
>  	hyp_vcpu->vcpu.arch.cflags = READ_ONCE(host_vcpu->arch.cflags);
>  	hyp_vcpu->vcpu.arch.mp_state.mp_state = KVM_MP_STATE_STOPPED;
>  
> +	if (pkvm_hyp_vcpu_is_protected(hyp_vcpu))
> +		kvm_init_pvm_id_regs(&hyp_vcpu->vcpu);
> +
>  	ret = pkvm_vcpu_init_traps(hyp_vcpu);
>  	if (ret)
>  		goto done;
> diff --git a/arch/arm64/kvm/hyp/nvhe/sys_regs.c b/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> index 59fb2f056177..7008e9641f41 100644
> --- a/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> +++ b/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> @@ -204,8 +204,7 @@ static u64 get_pvm_id_aa64mmfr2(const struct kvm_vcpu *vcpu)
>  	return id_aa64mmfr2_el1_sys_val & PVM_ID_AA64MMFR2_ALLOW;
>  }
>  
> -/* Read a sanitized cpufeature ID register by its encoding */
> -u64 pvm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
> +static u64 pvm_calc_id_reg(const struct kvm_vcpu *vcpu, u32 id)
>  {
>  	switch (id) {
>  	case SYS_ID_AA64PFR0_EL1:
> @@ -240,10 +239,25 @@ u64 pvm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
>  	}
>  }
>  
> +/* Read a sanitized cpufeature ID register by its encoding */
> +u64 pvm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
> +{
> +	return pvm_calc_id_reg(vcpu, id);
> +}
> +

Whatever reason you have to keep this per-CPU, it really needs to
change to be VM-scoped. Maybe not immediately, but that should be on
your list.

>  static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>  		       struct sys_reg_desc const *r)
>  {
> -	return pvm_read_id_reg(vcpu, reg_to_encoding(r));
> +	struct kvm *kvm = vcpu->kvm;
> +	u32 reg = reg_to_encoding(r);
> +
> +	if (WARN_ON_ONCE(!test_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags)))
> +		return 0;
> +
> +	if (reg >= sys_reg(3, 0, 0, 4, 0) && reg <= sys_reg(3, 0, 0, 7, 7))
> +		return kvm->arch.id_regs[IDREG_IDX(reg)];

Why the limit to the AA64 idregs? Aren't the other shadow sysreg
values correctly initialised to 0?

> +
> +	return 0;
>  }
>  
>  /* Handler to RAZ/WI sysregs */
> @@ -448,6 +462,40 @@ static const struct sys_reg_desc pvm_sys_reg_descs[] = {
>  	/* Performance Monitoring Registers are restricted. */
>  };
>  
> +/*
> + * Initializes feature registers for protected vms.
> + */
> +void kvm_init_pvm_id_regs(struct kvm_vcpu *vcpu)
> +{
> +	const u32 pvm_feat_id_regs[] = {

static?

> +		SYS_ID_AA64PFR0_EL1,
> +		SYS_ID_AA64PFR1_EL1,
> +		SYS_ID_AA64ISAR0_EL1,
> +		SYS_ID_AA64ISAR1_EL1,
> +		SYS_ID_AA64ISAR2_EL1,
> +		SYS_ID_AA64ZFR0_EL1,
> +		SYS_ID_AA64MMFR0_EL1,
> +		SYS_ID_AA64MMFR1_EL1,
> +		SYS_ID_AA64MMFR2_EL1,
> +		SYS_ID_AA64MMFR4_EL1,
> +		SYS_ID_AA64DFR0_EL1,
> +	};
> +	struct kvm *kvm = vcpu->kvm;
> +	unsigned long i;
> +
> +	if (WARN_ON_ONCE(test_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags)))
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(pvm_feat_id_regs); i++) {

But do you really need this array? Can't you just iterate over the
idreg space and compute the value? Sure, that'd be a bit longer, but
at least you would define the idregs you care about in a single place.

> +		struct kvm_arch *ka = &kvm->arch;
> +		u32 reg = pvm_feat_id_regs[i];
> +
> +		ka->id_regs[IDREG_IDX(reg)] = pvm_calc_id_reg(vcpu, reg);
> +	}
> +
> +	set_bit(KVM_ARCH_FLAG_ID_REGS_INITIALIZED, &kvm->arch.flags);
> +}
> +
>  /*
>   * Checks that the sysreg table is unique and in-order.
>   *

Thanks,

	M.

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