Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230530134248.23998-1-cloudliang@tencent.com>
 <20230530134248.23998-4-cloudliang@tencent.com> <ZJybjcyiLQVkSHMC@google.com>
In-Reply-To: <ZJybjcyiLQVkSHMC@google.com>
From:   Jim Mattson <jmattson@google.com>
Date:   Wed, 28 Jun 2023 14:03:13 -0700
Message-ID: <CALMp9eQmux6cip=a+FkLpfmMz2zMuMD6bec-zO2or5HQGZ7Hsw@mail.gmail.com>
Subject: Re: [PATCH v2 3/8] KVM: selftests: Test Intel PMU architectural
 events on gp counters
To:     Sean Christopherson <seanjc@google.com>
Cc:     Jinrong Liang <ljr.kernel@gmail.com>,
        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

On Wed, Jun 28, 2023 at 1:44=E2=80=AFPM Sean Christopherson <seanjc@google.=
com> wrote:
>
> On Tue, May 30, 2023, Jinrong Liang wrote:
> > +/* Guest payload for any performance counter counting */
> > +#define NUM_BRANCHES 10
> > +
> > +static struct kvm_vm *pmu_vm_create_with_one_vcpu(struct kvm_vcpu **vc=
pu,
> > +                                               void *guest_code)
> > +{
> > +     struct kvm_vm *vm;
> > +
> > +     vm =3D vm_create_with_one_vcpu(vcpu, guest_code);
> > +     vm_init_descriptor_tables(vm);
> > +     vcpu_init_descriptor_tables(*vcpu);
> > +
> > +     return vm;
> > +}
> > +
> > +static uint64_t run_vcpu(struct kvm_vcpu *vcpu, uint64_t *ucall_arg)
> > +{
> > +     struct ucall uc;
> > +
> > +     vcpu_run(vcpu);
> > +     switch (get_ucall(vcpu, &uc)) {
> > +     case UCALL_SYNC:
> > +             *ucall_arg =3D uc.args[1];
> > +             break;
> > +     case UCALL_DONE:
> > +             break;
> > +     default:
> > +             TEST_ASSERT(false, "Unexpected exit: %s",
> > +                         exit_reason_str(vcpu->run->exit_reason));
>
> TEST_FAIL()
>
> > +     }
> > +     return uc.cmd;
> > +}
> > +
> > +static void intel_guest_run_arch_event(uint8_t version, uint8_t max_gp=
_num,
>
> Unless I'm mistaken, this isn't specific to arch events.  And with a bit =
of
> massaging, it doesn't need to be Intel specific.  Typically we try to avo=
id
> speculatively creating infrastructure, but in this case we *know* AMD has=
 vPMU
> support, and we *know* from KVM-Unit-Tests that accounting for the differ=
ences
> between MSRs on Intel vs. AMD is doable, so we should write code with an =
eye
> toward supporting both AMD and Intel.
>
> And then we can avoid having to prefix so many functions with "intel", e.=
g. this
> can be something like
>
>   static void guest_measure_loop()
>
> or whatever.
>
> > +                                    uint32_t ctr_base_msr, uint64_t ev=
t_code)
> > +{
> > +     uint32_t global_msr =3D MSR_CORE_PERF_GLOBAL_CTRL;
> > +     unsigned int i;
> > +
> > +     for (i =3D 0; i < max_gp_num; i++) {
> > +             wrmsr(ctr_base_msr + i, 0);
> > +             wrmsr(MSR_P6_EVNTSEL0 + i, EVENTSEL_OS | EVENTSEL_EN | ev=
t_code);
> > +             if (version > 1)
> > +                     wrmsr(global_msr, BIT_ULL(i));
> > +
> > +             __asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES})=
);
> > +
> > +             if (version > 1)
> > +                     wrmsr(global_msr, 0);
> > +
> > +             GUEST_SYNC(_rdpmc(i));
> > +     }
> > +
> > +     GUEST_DONE();
> > +}
> > +
> > +static void test_arch_events_cpuid(struct kvm_vcpu *vcpu, uint8_t evt_=
vector,
>
> "vector" is confusing, as "vector" usually refers to a vector number, e.g=
. for
> IRQs and exceptions.  This is the _length_ of a so called vector.  I vote=
 to ignore
> the SDM's use of "vector" in this case and instead call it something like
> arch_events_bitmap_size.  And then arch_events_unavailable_mask?
>
> > +                                uint8_t unavl_mask, uint8_t idx)
> > +{
> > +     struct kvm_cpuid_entry2 *entry;
> > +     uint32_t ctr_msr =3D MSR_IA32_PERFCTR0;
> > +     bool is_supported;
> > +     uint64_t counter_val =3D 0;
> > +
> > +     entry =3D vcpu_get_cpuid_entry(vcpu, 0xa);
> > +     entry->eax =3D (entry->eax & ~EVT_LEN_MASK) |
> > +             (evt_vector << EVT_LEN_OFS_BIT);
>
> EVT_LEN_OFS_BIT can be a KVM_x86_PROPERTY.  And please also add a helper =
to set
> properties, the whole point of the FEATURE and PROPERTY frameworks is to =
avoid
> open coding CPUID manipulations.  E.g.
>
> static inline void vcpu_set_cpuid_property(struct kvm_vcpu *vcpu,
>                                            struct kvm_x86_cpu_property pr=
operty,
>                                            uint32_t value)
> {
>         ...
> }
>
> > +     entry->ebx =3D (entry->ebx & ~EVENTS_MASK) | unavl_mask;
> > +     vcpu_set_cpuid(vcpu);
> > +
> > +     if (vcpu_get_msr(vcpu, MSR_IA32_PERF_CAPABILITIES) & PMU_CAP_FW_W=
RITES)
> > +             ctr_msr =3D MSR_IA32_PMC0;
>
> This can be done in the guest, no?
>
> > +
> > +     /* Arch event x is supported if EBX[x]=3D0 && EAX[31:24]>x */
> > +     is_supported =3D !(entry->ebx & BIT_ULL(idx)) &&
> > +             (((entry->eax & EVT_LEN_MASK) >> EVT_LEN_OFS_BIT) > idx);
>
> Please add a helper for this.
>
> > +
> > +     vcpu_args_set(vcpu, 4, X86_INTEL_PMU_VERSION, X86_INTEL_MAX_GP_CT=
R_NUM,
> > +                   ctr_msr, arch_events[idx]);
> > +
> > +     while (run_vcpu(vcpu, &counter_val) !=3D UCALL_DONE)
> > +             TEST_ASSERT(is_supported =3D=3D !!counter_val,
> > +                         "Unavailable arch event is counting.");
> > +}
> > +
> > +static void intel_check_arch_event_is_unavl(uint8_t idx)
> > +{
> > +     uint8_t eax_evt_vec, ebx_unavl_mask, i, j;
> > +     struct kvm_vcpu *vcpu;
> > +     struct kvm_vm *vm;
> > +
> > +     /*
> > +      * A brute force iteration of all combinations of values is likel=
y to
> > +      * exhaust the limit of the single-threaded thread fd nums, so it=
's
> > +      * tested here by iterating through all valid values on a single =
bit.
> > +      */
> > +     for (i =3D 0; i < ARRAY_SIZE(arch_events); i++) {
> > +             eax_evt_vec =3D BIT_ULL(i);
> > +             for (j =3D 0; j < ARRAY_SIZE(arch_events); j++) {
> > +                     ebx_unavl_mask =3D BIT_ULL(j);
> > +                     vm =3D pmu_vm_create_with_one_vcpu(&vcpu,
> > +                                                      intel_guest_run_=
arch_event);
> > +                     test_arch_events_cpuid(vcpu, eax_evt_vec,
> > +                                            ebx_unavl_mask, idx);
> > +
> > +                     kvm_vm_free(vm);
> > +             }
> > +     }
> > +}
> > +
> > +static void intel_test_arch_events(void)
> > +{
> > +     uint8_t idx;
> > +
> > +     for (idx =3D 0; idx < ARRAY_SIZE(arch_events); idx++) {
> > +             /*
> > +              * Given the stability of performance event recurrence,
> > +              * only these arch events are currently being tested:
> > +              *
> > +              * - Core cycle event (idx =3D 0)
> > +              * - Instruction retired event (idx =3D 1)
> > +              * - Reference cycles event (idx =3D 2)
> > +              * - Branch instruction retired event (idx =3D 5)
> > +              *
> > +              * Note that reference cycles is one event that actually =
cannot
> > +              * be successfully virtualized.
> > +              */

Actually, there is no reason that reference cycles can't be
successfully virtualized. It just can't be done with the current vPMU
infrastructure.

> > +             if (idx > 2 && idx !=3D 5)
>
> As request in a previous patch, use enums, then the need to document the =
magic
> numbers goes away.
>
> > +                     continue;
> > +
> > +             intel_check_arch_event_is_unavl(idx);
> > +     }
> > +}
> > +
> > +static void intel_test_pmu_cpuid(void)
> > +{
> > +     intel_test_arch_events();
>
> Either put the Intel-specific TEST_REQUIRE()s in here, or open code the c=
alls.
> Adding a helper and then splitting code across the helper and its sole ca=
ller is
> unnecessary.
>
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +     TEST_REQUIRE(get_kvm_param_bool("enable_pmu"));
> > +
> > +     if (host_cpu_is_intel) {
>
> Presumably AMD will be supported at some point, but until then, this need=
s to be
>
>         TEST_REQUIRE(host_cpu_is_intel);
>
> > +             TEST_REQUIRE(kvm_cpu_has_p(X86_PROPERTY_PMU_VERSION));
> > +             TEST_REQUIRE(X86_INTEL_PMU_VERSION > 0);
> > +             TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_PDCM));
> > +
> > +             intel_test_pmu_cpuid();
> > +     }
> > +
> > +     return 0;
> > +}
> > --
> > 2.31.1
> >