Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
MIME-Version: 1.0
References: <20230911114347.85882-1-cloudliang@tencent.com>
 <20230911114347.85882-9-cloudliang@tencent.com> <ZTLPy9SYzJmgMxw9@google.com>
In-Reply-To: <ZTLPy9SYzJmgMxw9@google.com>
From:   Jinrong Liang <ljr.kernel@gmail.com>
Date:   Sat, 21 Oct 2023 17:58:14 +0800
Message-ID: <CAFg_LQXZ4k+K7paWXVFSpXvaqBY-2s1DNh=t91-pRBc8efwt9w@mail.gmail.com>
Subject: Re: [PATCH v4 8/9] KVM: selftests: Test Intel supported fixed
 counters bit mask
To:     Sean Christopherson <seanjc@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, Like Xu <likexu@tencent.com>,
        David Matlack <dmatlack@google.com>,
        Aaron Lewis <aaronlewis@google.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jinrong Liang <cloudliang@tencent.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Sean Christopherson <seanjc@google.com> =E4=BA=8E2023=E5=B9=B410=E6=9C=8821=
=E6=97=A5=E5=91=A8=E5=85=AD 03:06=E5=86=99=E9=81=93=EF=BC=9A
>
> On Mon, Sep 11, 2023, Jinrong Liang wrote:
> > From: Jinrong Liang <cloudliang@tencent.com>
> >
> > Add a test to check that fixed counters enabled via guest
> > CPUID.0xA.ECX (instead of EDX[04:00]) work as normal as usual.
> >
> > Co-developed-by: Like Xu <likexu@tencent.com>
> > Signed-off-by: Like Xu <likexu@tencent.com>
> > Signed-off-by: Jinrong Liang <cloudliang@tencent.com>
> > ---
> >  .../selftests/kvm/x86_64/pmu_counters_test.c  | 54 +++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/t=
ools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> > index df76f0f2bfd0..12c00bf94683 100644
> > --- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c
> > @@ -301,6 +301,59 @@ static void test_intel_counters_num(void)
> >       test_oob_fixed_ctr(nr_fixed_counters + 1);
> >  }
> >
> > +static void fixed_counters_guest_code(void)
> > +{
> > +     uint64_t supported_bitmask =3D this_cpu_property(X86_PROPERTY_PMU=
_FIXED_COUNTERS_BITMASK);
> > +     uint32_t nr_fixed_counter =3D this_cpu_property(X86_PROPERTY_PMU_=
NR_FIXED_COUNTERS);
> > +     uint64_t msr_val;
> > +     unsigned int i;
> > +     bool expected;
> > +
> > +     for (i =3D 0; i < nr_fixed_counter; i++) {
> > +             expected =3D supported_bitmask & BIT_ULL(i) || i < nr_fix=
ed_counter;
> > +
> > +             wrmsr_safe(MSR_CORE_PERF_FIXED_CTR0 + i, 0);
> > +             wrmsr_safe(MSR_CORE_PERF_FIXED_CTR_CTRL, BIT_ULL(4 * i));
> > +             wrmsr_safe(MSR_CORE_PERF_GLOBAL_CTRL, BIT_ULL(PMC_IDX_FIX=
ED + i));
> > +             __asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES})=
);
> > +             wrmsr_safe(MSR_CORE_PERF_GLOBAL_CTRL, 0);
> > +             rdmsr_safe(MSR_CORE_PERF_FIXED_CTR0 + i, &msr_val);
>
> Y'all are making this way harder than it needs to be.  The previous patch=
 already
> created a testcase to verify fixed counters, just use that!  Then test ca=
se verify
> that trying to enable unsupported fixed counters results in #GP, as oppos=
ed to the
> above which doesn't do any actual checking, e.g. KVM could completely bot=
ch the
> {RD,WR}MSR emulation but pass the test by not programming up a counter in=
 perf.
>
> I.e. rather than have a separate test for the supported bitmask goofiness=
, have
> the fixed counters test iterate over the bitmask.  And then add a patch t=
o verify
> the counters can be enabled and actually count.
>
> And peeking ahead at the vPMU version test, it's the exact same story the=
re.
> Instead of hardcoding one-off tests, iterate on the version.  The end res=
ult is
> that the test provides _more_ coverage with _less_ code.  And without any=
 of the
> hardcoded magic that takes a crystal ball to understand.
>
> *sigh*
>
> And even more importantly, this test is complete garbage.  The SDM clearl=
y states
> that
>
>   With Architectural Performance Monitoring Version 5, register CPUID.0AH=
.ECX
>   indicates Fixed Counter enumeration. It is a bit mask which enumerates =
the
>   supported Fixed Counters in a processor. If bit 'i' is set, it implies =
that
>   Fixed Counter 'i' is supported.
>
> *sigh*
>
> The test passes because it only iterates over counters < nr_fixed_counter=
.  So
> as written, the test worse than useless.  It provides no meaningful value=
 and is
> actively misleading.
>
>         for (i =3D 0; i < nr_fixed_counter; i++) {
>
> Maybe I haven't been explicit enough: the point of writing tests is to fi=
nd and
> prevent bugs, not to get the tests passing.  That isn't to say we don't w=
ant a
> clean testgrid, but writing a "test" that doesn't actually test anything =
is a
> waste of everyone's time.
>
> I appreciate that the PMU is subtle and complex (understatement), but thi=
ngs like
> this, where observing that the result of "supported_bitmask & BIT_ULL(i)"=
 doesn't
> actually affect anything, doesn't require PMU knowledge.
>
>         for (i =3D 0; i < nr_fixed_counter; i++) {
>                 expected =3D supported_bitmask & BIT_ULL(i) || i < nr_fix=
ed_counter;
>
> A concrete suggestion for writing tests: introduce bugs in what you're te=
sting
> and verify that the test actually detects the bugs.  If you tried to do t=
hat for
> the above bitmask test you would have discovered you can't break KVM beca=
use KVM
> doesn't support this!  And if your test doesn't detect the bugs, that sho=
uld also
> be a big clue that something isn't quite right.

Thank you for your detailed feedback on my patch series. I truly
appreciate the time and effort you've put into identifying the issues
in my code and providing valuable suggestions for improvement.

Your guidance have been instrumental in helping me understand the
selftests. I will make sure to strive to create more meaningful and
effective contribution in the future.