Re: [PATCH 1/3] KVM: arm64: pkvm: Actually enable/disable PMU events when running vCPU

[Marc Zyngier <maz@kernel.org>]

On Tue, 05 Mar 2024 18:48:38 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> flush_hyp_vcpu() does not carry through the PMU event context from the
> host, regardless of whether the VM is protected or not. This is
> problematic for two reasons:
> 
>  - The expectation for non-protected VMs is that the protected-mode
>    hypervisor has feature parity with the 'normal' KVM configuration.
>    However, PMU events programmed to run with the guest do not work.
> 
>  - The protected-mode hypervisor needs to prevent the host from peering
>    in on the guest. Nothing stops the host from leaving an event enabled
>    straight past guest entry...
> 
> Address the both of these issues by priming the hyp vCPU's PMU event
> context before entering the guest. Take special care to not trust the
> host in the case of pVMs by referring directly to what hardware says is
> enabled.
> 
> Cc: Will Deacon <will@kernel.org>
> Fixes: be66e67f1750 ("KVM: arm64: Use the pKVM hyp vCPU structure in handle___kvm_vcpu_run()")
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/hyp/nvhe/hyp-main.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 2385fd03ed87..8621f8383f0c 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -23,6 +23,26 @@ DEFINE_PER_CPU(struct kvm_nvhe_init_params, kvm_init_params);
>  
>  void __kvm_hyp_host_forward_smc(struct kvm_cpu_context *host_ctxt);
>  
> +static void flush_debug_state(struct pkvm_hyp_vcpu *hyp_vcpu)
> +{
> +	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> +
> +	hyp_vcpu->vcpu.arch.mdcr_el2	= host_vcpu->arch.mdcr_el2;
> +	hyp_vcpu->vcpu.arch.debug_ptr	= kern_hyp_va(host_vcpu->arch.debug_ptr);
> +
> +	/*
> +	 * The host's PMU context cannot be trusted for protected VMs. Refer to
> +	 * hardware to determine which PMCs need to be disabled/enabled on vCPU
> +	 * entry/exit, respectively.
> +	 */
> +	if (kvm_vm_is_protected(kern_hyp_va(host_vcpu->kvm))) {

This looks wrong. We should never rely on *host* controlled data to
make a decision at EL2 in the pKVM case. Use of this helper at EL2
should be constrained to the hyp view of the kvm structure only.

The other thing is that most of this code is going to be thrown away,
hopefully soon... (hint, nudge...).

Thanks,

	M.

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