Re: [PATCH] KVM: arm64: Advertise ID_AA64PFR0_EL1.CSV2/3 to protected VMs

[Marc Zyngier <maz@kernel.org>]

On Wed, 29 Mar 2023 13:15:26 +0100,
Fuad Tabba <tabba@google.com> wrote:
> 
> The existing pKVM code attempts to do that using a value that's
> initialized to 0 but never set. To advertise csv2/3 to a
> protected guest, store them at the hypervisor and use them for
> setting csv2/3.
> 
> Similar to non-protected KVM, these are tracked as a system-wide
> variable, rather than per cpu, for simplicity.
> 
> Fixes: 6c30bfb18d0b ("KVM: arm64: Add handlers for protected VM System Registers")
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  arch/arm64/include/asm/kvm_hyp.h   |  3 +++
>  arch/arm64/kvm/arm.c               |  2 ++
>  arch/arm64/kvm/hyp/nvhe/sys_regs.c | 15 ++++++++++++---
>  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index bdd9cf546d95..723a645af191 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -127,4 +127,7 @@ extern u64 kvm_nvhe_sym(id_aa64smfr0_el1_sys_val);
>  extern unsigned long kvm_nvhe_sym(__icache_flags);
>  extern unsigned int kvm_nvhe_sym(kvm_arm_vmid_bits);
>  
> +extern bool kvm_nvhe_sym(spectre_unaffected);
> +extern bool kvm_nvhe_sym(meltdown_unaffected);
> +
>  #endif /* __ARM64_KVM_HYP_H__ */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 3bd732eaf087..364a4440ae54 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1902,6 +1902,8 @@ static void kvm_hyp_init_symbols(void)
>  	kvm_nvhe_sym(id_aa64smfr0_el1_sys_val) = read_sanitised_ftr_reg(SYS_ID_AA64SMFR0_EL1);
>  	kvm_nvhe_sym(__icache_flags) = __icache_flags;
>  	kvm_nvhe_sym(kvm_arm_vmid_bits) = kvm_arm_vmid_bits;
> +	kvm_nvhe_sym(spectre_unaffected) = (arm64_get_spectre_v2_state() == SPECTRE_UNAFFECTED);
> +	kvm_nvhe_sym(meltdown_unaffected) = (arm64_get_meltdown_state() == SPECTRE_UNAFFECTED);
>  }
>  
>  static int __init kvm_hyp_init_protection(u32 hyp_va_bits)
> diff --git a/arch/arm64/kvm/hyp/nvhe/sys_regs.c b/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> index 08d2b004f4b7..3f647a2f4c96 100644
> --- a/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> +++ b/arch/arm64/kvm/hyp/nvhe/sys_regs.c
> @@ -28,6 +28,16 @@ u64 id_aa64mmfr1_el1_sys_val;
>  u64 id_aa64mmfr2_el1_sys_val;
>  u64 id_aa64smfr0_el1_sys_val;
>  
> +/*
> + * Track whether the system isn't affected by spectre/meltown.
> + * Although this is per-CPU, we make it global for simplicity, e.g., not to have
> + * to worry about migration.
> + *
> + * Unlike for non-protected VMs, userspace cannot override this.
> + */
> +bool spectre_unaffected;
> +bool meltdown_unaffected;
> +
>  /*
>   * Inject an unknown/undefined exception to an AArch64 guest while most of its
>   * sysregs are live.
> @@ -85,7 +95,6 @@ static u64 get_restricted_features_unsigned(u64 sys_reg_val,
>  
>  static u64 get_pvm_id_aa64pfr0(const struct kvm_vcpu *vcpu)
>  {
> -	const struct kvm *kvm = (const struct kvm *)kern_hyp_va(vcpu->kvm);
>  	u64 set_mask = 0;
>  	u64 allow_mask = PVM_ID_AA64PFR0_ALLOW;
>  
> @@ -94,9 +103,9 @@ static u64 get_pvm_id_aa64pfr0(const struct kvm_vcpu *vcpu)
>  
>  	/* Spectre and Meltdown mitigation in KVM */
>  	set_mask |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV2),
> -			       (u64)kvm->arch.pfr0_csv2);
> +			       spectre_unaffected);
>  	set_mask |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_CSV3),
> -			       (u64)kvm->arch.pfr0_csv3);
> +			       meltdown_unaffected);

Since you already have a sanitised version of ID_AA64PFR0_EL1, it
would seem more straightforward to directly perform this adjustment at
the point where you export the value, rather than doing it at runtime.

Ideally, the proton-pack code would perform the update and set the
sanitised values directly in the cpufeature repository, but this would
involve me looking at it again, and I really don't want to do that.

Thanks,

	M.

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