Received-SPF: pass (google.com: domain of linux-kernel+bounces-91188-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 4 Mar 2024 11:46:24 -0800
In-Reply-To: <f5bbe9ac-ca35-4c3e-8cd7-249839fbb8b8@linux.intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240301075007.644152-1-sandipan.das@amd.com> <06061a28-88c0-404b-98a6-83cc6cc8c796@gmail.com>
 <cc8699be-3aae-42aa-9c70-f8b6a9728ee3@amd.com> <f5bbe9ac-ca35-4c3e-8cd7-249839fbb8b8@linux.intel.com>
Message-ID: <ZeYlEGORqeTPLK2_@google.com>
Subject: Re: [PATCH] KVM: x86/svm/pmu: Set PerfMonV2 global control bits correctly
From: Sean Christopherson <seanjc@google.com>
To: Dapeng Mi <dapeng1.mi@linux.intel.com>
Cc: Sandipan Das <sandipan.das@amd.com>, Like Xu <like.xu.linux@gmail.com>, pbonzini@redhat.com, 
	mizhang@google.com, jmattson@google.com, ravi.bangoria@amd.com, 
	nikunj.dadhania@amd.com, santosh.shukla@amd.com, manali.shukla@amd.com, 
	babu.moger@amd.com, kvm list <kvm@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Mar 04, 2024, Dapeng Mi wrote:
>=20
> On 3/1/2024 5:00 PM, Sandipan Das wrote:
> > On 3/1/2024 2:07 PM, Like Xu wrote:
> > > On 1/3/2024 3:50 pm, Sandipan Das wrote:
> > > > With PerfMonV2, a performance monitoring counter will start operati=
ng
> > > > only when both the PERF_CTLx enable bit as well as the correspondin=
g
> > > > PerfCntrGlobalCtl enable bit are set.
> > > >=20
> > > > When the PerfMonV2 CPUID feature bit (leaf 0x80000022 EAX bit 0) is=
 set
> > > > for a guest but the guest kernel does not support PerfMonV2 (such a=
s
> > > > kernels older than v5.19), the guest counters do not count since th=
e
> > > > PerfCntrGlobalCtl MSR is initialized to zero and the guest kernel n=
ever
> > > > writes to it.
> > > If the vcpu has the PerfMonV2 feature, it should not work the way leg=
acy
> > > PMU does. Users need to use the new driver to operate the new hardwar=
e,
> > > don't they ? One practical approach is that the hypervisor should not=
 set
> > > the PerfMonV2 bit for this unpatched 'v5.19' guest.
> > >=20
> > My understanding is that the legacy method of managing the counters sho=
uld
> > still work because the enable bits in PerfCntrGlobalCtl are expected to=
 be
> > set. The AMD PPR does mention that the PerfCntrEn bitfield of PerfCntrG=
lobalCtl
> > is set to 0x3f after a system reset. That way, the guest kernel can use=
 either
>=20
> If so, please add the PPR description here as comments.

Or even better, make that architectural behavior that's documented in the A=
PM.

> > > > ---
> > > >  =C2=A0 arch/x86/kvm/svm/pmu.c | 1 +
> > > >  =C2=A0 1 file changed, 1 insertion(+)
> > > >=20
> > > > diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
> > > > index b6a7ad4d6914..14709c564d6a 100644
> > > > --- a/arch/x86/kvm/svm/pmu.c
> > > > +++ b/arch/x86/kvm/svm/pmu.c
> > > > @@ -205,6 +205,7 @@ static void amd_pmu_refresh(struct kvm_vcpu *vc=
pu)
> > > >  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (pmu->version > 1) {
> > > >  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pmu->global=
_ctrl_mask =3D ~((1ull << pmu->nr_arch_gp_counters) - 1);
> > > >  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pmu->global=
_status_mask =3D pmu->global_ctrl_mask;
> > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pmu->global_ctrl =3D ~p=
mu->global_ctrl_mask;
>=20
> It seems to be more easily understand to calculate global_ctrl firstly an=
d
> then derive the globol_ctrl_mask (negative logic).

Hrm, I'm torn.  On one hand, awful name aside (global_ctrl_mask should real=
ly be
something like global_ctrl_rsvd_bits), the computation of the reserved bits=
 should
come from the capabilities of the PMU, not from the RESET value.

On the other hand, setting _all_ non-reserved bits will likely do the wrong=
 thing
if AMD ever adds bits in PerfCntGlobalCtl that aren't tied to general purpo=
se
counters.  But, that's a future theoretical problem, so I'm inclined to vot=
e for
Sandipan's approach.

> diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
> index e886300f0f97..7ac9b080aba6 100644
> --- a/arch/x86/kvm/svm/pmu.c
> +++ b/arch/x86/kvm/svm/pmu.c
> @@ -199,7 +199,8 @@ static void amd_pmu_refresh(struct kvm_vcpu *vcpu)
> kvm_pmu_cap.num_counters_gp);
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (pmu->version > 1) {
> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 pmu->global_ctrl_mask =3D ~((1ull << pmu->nr_arch_gp_counters)
> - 1);
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 pmu->global_ctrl =3D (1ull << pmu->nr_arch_gp_counters) - 1;
> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 pmu->global_ctrl_mask =3D ~pmu->global_ctrl;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0 pmu->global_status_mask =3D pmu->global_ctrl_mask;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
>=20
> > > >  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 }
> > > >  =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pmu->counter_bitmask[KVM_PMC=
_GP] =3D ((u64)1 << 48) - 1;
> >=20