Re: [PATCH v1] KVM: arm64: PMU: Restore the guest's EL0 event counting after migration

From: Marc Zyngier <maz@kernel.org>
To: Reiji Watanabe <reijiw@google.com>
Cc: Oliver Upton <oliver.upton@linux.dev>,
	kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Ricardo Koller <ricarkol@google.com>,
	Jing Zhang <jingzhangos@google.com>,
	Raghavendra Rao Anata <rananta@google.com>,
	Will Deacon <will@kernel.org>,
	stable@vger.kernel.org
Subject: Re: [PATCH v1] KVM: arm64: PMU: Restore the guest's EL0 event counting after migration
Date: Tue, 28 Mar 2023 12:08:31 +0100	[thread overview]
Message-ID: <86pm8tw2ts.wl-maz@kernel.org> (raw)
In-Reply-To: <20230328034725.2051499-1-reijiw@google.com>

On Tue, 28 Mar 2023 04:47:25 +0100,
Reiji Watanabe <reijiw@google.com> wrote:
> 
> Currently, with VHE, KVM enables the EL0 event counting for the
> guest on vcpu_load() or KVM enables it as a part of the PMU
> register emulation process, when needed.  However, in the migration
> case (with VHE), the same handling is lacking.  So, enable it on the
> first KVM_RUN with VHE (after the migration) when needed.

It wasn't completely clear to me how the migration case was affected
by this until I started digging into the call stack:

At load-time, the PMCR_EL0 effects haven't been propagated yet (the
events haven't been created, as this is what kvm_pmu_handle_pmcr()
does on first run). So there is an ordering inversion between
kvm_pmu_handle_pmcr() and kvm_vcpu_pmu_restore_guest().

Moving the latter call into the former fixes the issue, completely
emulating an extra write to PMCR_EL0.

I think it would be worth capturing some of the above in the commit
message so that it doesn't get lost...

> 
> Fixes: d0c94c49792c ("KVM: arm64: Restore PMU configuration on first run")
> Cc: stable@vger.kernel.org
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> ---
>  arch/arm64/kvm/pmu-emul.c | 1 +
>  arch/arm64/kvm/sys_regs.c | 1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index c243b10f3e15..5eca0cdd961d 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -558,6 +558,7 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)
>  		for_each_set_bit(i, &mask, 32)
>  			kvm_pmu_set_pmc_value(kvm_vcpu_idx_to_pmc(vcpu, i), 0, true);
>  	}
> +	kvm_vcpu_pmu_restore_guest(vcpu);
>  }
>  
>  static bool kvm_pmu_counter_is_enabled(struct kvm_pmc *pmc)
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 1b2c161120be..34688918c811 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -794,7 +794,6 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  		if (!kvm_supports_32bit_el0())
>  			val |= ARMV8_PMU_PMCR_LC;
>  		kvm_pmu_handle_pmcr(vcpu, val);
> -		kvm_vcpu_pmu_restore_guest(vcpu);
>  	} else {
>  		/* PMCR.P & PMCR.C are RAZ */
>  		val = __vcpu_sys_reg(vcpu, PMCR_EL0)

With the nitpicking above addressed, and should this go into 6.3 as a
fix:

Reviewed-by: Marc Zyngier <maz@kernel.org>

I can otherwise take it into 6.4, depending on what Oliver decides to
do.

Thanks,

	M.

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