[PATCH v4 2/6] KVM: arm64: Save ID registers' sanitized value per guest

[Marc Zyngier]

On Fri, 17 Mar 2023 05:06:33 +0000,
Jing Zhang <jingzhangos at google.com> wrote:
> 
> From: Reiji Watanabe <reijiw at google.com>
> 
> Introduce id_regs[] in kvm_arch as a storage of guest's ID registers,
> and save ID registers' sanitized value in the array at KVM_CREATE_VM.
> Use the saved ones when ID registers are read by the guest or
> userspace (via KVM_GET_ONE_REG).
> 
> No functional change intended.
> 
> Signed-off-by: Reiji Watanabe <reijiw at google.com>
> Co-developed-by: Jing Zhang <jingzhangos at google.com>
> Signed-off-by: Jing Zhang <jingzhangos at google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 11 ++++++++
>  arch/arm64/kvm/arm.c              |  1 +
>  arch/arm64/kvm/id_regs.c          | 44 ++++++++++++++++++++++++-------
>  arch/arm64/kvm/sys_regs.c         |  2 +-
>  arch/arm64/kvm/sys_regs.h         |  1 +
>  5 files changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index a1892a8f6032..fb6b50b1f111 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -245,6 +245,15 @@ struct kvm_arch {
>  	 * the associated pKVM instance in the hypervisor.
>  	 */
>  	struct kvm_protected_vm pkvm;
> +
> +	/*
> +	 * Save ID registers for the guest in id_regs[].
> +	 * (Op0, Op1, CRn, CRm, Op2) of the ID registers to be saved in it
> +	 * is (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
> +	 */
> +#define KVM_ARM_ID_REG_NUM	56
> +#define IDREG_IDX(id)		(((sys_reg_CRm(id) - 1) << 3) | sys_reg_Op2(id))
> +	u64 id_regs[KVM_ARM_ID_REG_NUM];

Place these registers in their own structure, and place this structure
*before* the pvm structure. Document what guards these registers when
updated (my hunch is that this should rely on Oliver's locking fixes
if the update comes from a vcpu).

>  };
>  
>  struct kvm_vcpu_fault_info {
> @@ -1005,6 +1014,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
>  long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
>  				struct kvm_arm_copy_mte_tags *copy_tags);
>  
> +void kvm_arm_set_default_id_regs(struct kvm *kvm);
> +
>  /* Guest/host FPSIMD coordination helpers */
>  int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
>  void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 3bd732eaf087..4579c878ab30 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -153,6 +153,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  
>  	set_default_spectre(kvm);
>  	kvm_arm_init_hypercalls(kvm);
> +	kvm_arm_set_default_id_regs(kvm);
>  
>  	/*
>  	 * Initialise the default PMUver before there is a chance to
> diff --git a/arch/arm64/kvm/id_regs.c b/arch/arm64/kvm/id_regs.c
> index 08b738852955..e393b5730557 100644
> --- a/arch/arm64/kvm/id_regs.c
> +++ b/arch/arm64/kvm/id_regs.c
> @@ -52,16 +52,9 @@ static u8 pmuver_to_perfmon(u8 pmuver)
>  	}
>  }
>  
> -/* Read a sanitised cpufeature ID register by sys_reg_desc */
> -static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
> +u64 kvm_arm_read_id_reg(const struct kvm_vcpu *vcpu, u32 id)
>  {
> -	u32 id = reg_to_encoding(r);
> -	u64 val;
> -
> -	if (sysreg_visible_as_raz(vcpu, r))
> -		return 0;
> -
> -	val = read_sanitised_ftr_reg(id);
> +	u64 val = vcpu->kvm->arch.id_regs[IDREG_IDX(id)];
>  
>  	switch (id) {
>  	case SYS_ID_AA64PFR0_EL1:
> @@ -126,6 +119,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r
>  	return val;
>  }
>  
> +static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r)
> +{
> +	if (sysreg_visible_as_raz(vcpu, r))
> +		return 0;
> +
> +	return kvm_arm_read_id_reg(vcpu, reg_to_encoding(r));
> +}
> +
>  /* cpufeature ID register access trap handlers */
>  
>  static bool access_id_reg(struct kvm_vcpu *vcpu,
> @@ -504,3 +505,28 @@ int kvm_arm_walk_id_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
>  	}
>  	return total;
>  }
> +
> +/*
> + * Set the guest's ID registers that are defined in id_reg_descs[]
> + * with ID_SANITISED() to the host's sanitized value.
> + */
> +void kvm_arm_set_default_id_regs(struct kvm *kvm)
> +{
> +	int i;
> +	u32 id;
> +	u64 val;
> +
> +	for (i = 0; i < ARRAY_SIZE(id_reg_descs); i++) {
> +		id = reg_to_encoding(&id_reg_descs[i]);
> +		if (WARN_ON_ONCE(!is_id_reg(id)))
> +			/* Shouldn't happen */
> +			continue;
> +
> +		if (id_reg_descs[i].visibility == raz_visibility)
> +			/* Hidden or reserved ID register */
> +			continue;

Relying on function pointer comparison is really fragile. If I wrap
raz_visibility() in another function, this won't catch it. It also
doesn't bode well with your 'inline' definition of this function.

More importantly, why do we care about checking for visibility at all?
We can happily populate the array and rely on the runtime visibility.

> +
> +		val = read_sanitised_ftr_reg(id);
> +		kvm->arch.id_regs[IDREG_IDX(id)] = val;
> +	}
> +}

Thanks,

	M.

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