Re: [PATCH] KVM: arm64: pmu: Resync EL0 state on counter rotation

From: Leo Yan <leo.yan@linaro.org>
To: Shijie Huang <shijie@amperemail.onmicrosoft.com>
Cc: Marc Zyngier <maz@kernel.org>,
	kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	kvm@vger.kernel.org, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Oliver Upton <oliver.upton@linux.dev>,
	Zenghui Yu <yuzenghui@huawei.com>,
	Huang Shijie <shijie@os.amperecomputing.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Will Deacon <will@kernel.org>
Subject: Re: [PATCH] KVM: arm64: pmu: Resync EL0 state on counter rotation
Date: Mon, 14 Aug 2023 16:47:10 +0800	[thread overview]
Message-ID: <20230814084710.GA69080@leoy-huanghe.lan> (raw)
In-Reply-To: <5608d22d-47c3-2a03-a3d9-ba8ec51679a3@amperemail.onmicrosoft.com>

Hi Shijie,

On Mon, Aug 14, 2023 at 04:12:23PM +0800, Shijie Huang wrote:

[...]

> > > Their investigation indicates that upon counter rotation (which
> > > takes place on the back of a timer interrupt), we fail to
> > > re-apply the guest EL0 enabling, leading to the counting of host
> > > events instead of guest events.
> >
> > Seems to me, it's not clear for why the counter rotation will cause
> > the issue.
> > 
> > In the example shared by Shijie in [1], the cycle counter is enabled for
> > both host and guest, and cycle counter is a dedicated event which does
> > not share counter with other events.  Even there have counter rotation,
> > it should not impact the cycle counter.
> 
> Just take a simple case:
> 
>    perf stat -e cycles:G,cycles:H, e2,e3,e4,e5,e6,e7 ....
> 
> 
> Assume we have 8 events, but PMU only privides 7 counters(cycle + 6 normal)

Thanks for the detailed info, now I understand it.

Seems to me, based on Marc's patch, we need to apply below change.  In
below code, we don't need to change the perf core code and we can
resolve it as a common issue for Arm PMU drivers.

diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
index 121f1a14c829..8f9673cdadec 100644
--- a/arch/arm64/kvm/pmu.c
+++ b/arch/arm64/kvm/pmu.c
@@ -38,14 +38,20 @@ struct kvm_pmu_events *kvm_get_pmu_events(void)
 void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr)
 {
 	struct kvm_pmu_events *pmu = kvm_get_pmu_events();
+	int resync;
 
 	if (!kvm_arm_support_pmu_v3() || !pmu || !kvm_pmu_switch_needed(attr))
 		return;
 
+	resync = pmu->events_guest != set;
+
 	if (!attr->exclude_host)
 		pmu->events_host |= set;
 	if (!attr->exclude_guest)
 		pmu->events_guest |= set;
+
+	if (resync)
+		kvm_vcpu_pmu_resync_el0();
 }
 
 /*
@@ -60,6 +66,8 @@ void kvm_clr_pmu_events(u32 clr)
 
 	pmu->events_host &= ~clr;
 	pmu->events_guest &= ~clr;
+
+	kvm_vcpu_pmu_resync_el0();
 }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
