From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1DBE8C00A98 for ; Thu, 19 Oct 2023 23:55:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346729AbjJSXzf (ORCPT ); Thu, 19 Oct 2023 19:55:35 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40440 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235572AbjJSXze (ORCPT ); Thu, 19 Oct 2023 19:55:34 -0400 Received: from mail-yb1-xb4a.google.com (mail-yb1-xb4a.google.com [IPv6:2607:f8b0:4864:20::b4a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D0DF7113 for ; Thu, 19 Oct 2023 16:55:28 -0700 (PDT) Received: by mail-yb1-xb4a.google.com with SMTP id 3f1490d57ef6-d9cb3a59a19so238562276.2 for ; Thu, 19 Oct 2023 16:55:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1697759728; x=1698364528; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=VT3/dh9UAKVu/RjZAMlDFfIDIF9ioyj3Z5qJzNl9A0E=; b=q7hX96F2Eba7a+r+lS2/QCVAGPaOaAVcCTFhOI6xzb0yRoO0Ci/pSATynAWpSZwqNZ AKuDG8uVfKmLYGmJ+VEQvmzgPP7xyIYTkgNNh+n8NUoOv41uYUMAiFOtafnVfuUE4eXF ro9h/4+6CpH1QER77q0j0eHmIHLhjjdOCeSDkwI7nV8CSA2qUPpXGcoIFk2tHRuq/Qyx g+geVjWfU56adJ/JgM93G0bb1uBepUG4rk3qAi44nW9JSbSGh/M6ByGRSCNxfD+CwgYl kNresMNT//hgv9Yg9PqLiSQ2YI7rUMS5mMzhtpz7q5IgomEToQw6DCvK0dv260kL/PL4 ihUg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697759728; x=1698364528; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=VT3/dh9UAKVu/RjZAMlDFfIDIF9ioyj3Z5qJzNl9A0E=; b=xDumkuZrEZs9JZLZU2L2IO9QWQyA0dCcJJAHLfFeYDMIkU7ZQZ3miCzYP01g4me8Vy xTVXAdtywUEQAw1jKOqn2tRXm2sRUj5pnCsZz6zpgJIvg6mnxicQ+J0JwcSr8FiVhr9Z 9bObWsvBpJNDaFyEIV/ER03AvB/q8OzwzcjfKlAZiCCnk9f3XX85cJRhXLf+/MAGsmaz EVR/v0F0+L9oYhK6dZD8GlgyLt6FoOEwW/GqZIh1WU6zCpxlZSV+AAPxG1HJlAgHSaLT tnVi4Z/5vVC4YX5k/+rZVFm/9hlxj/IqMLw5MHHoc0aZDzBMWWx2y7sJxMM9AfvYVHmE BYyA== X-Gm-Message-State: AOJu0YyEtmXO7f56SqnWApyD5u2r+BGMVOzs/gbdC/7ygODK/KCxbTkp 39ncCc6mkv3keOkok3FXeTGEDMt9ky8= X-Google-Smtp-Source: AGHT+IHZrJQjP9f5UAqT8B8i5/S+Wo6M6T0lWykyNkZvtfWkECs63EluSCkUjO3uzi/5x+WjjG7L2HusUKQ= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a05:6902:134a:b0:d9a:c588:38a with SMTP id g10-20020a056902134a00b00d9ac588038amr7876ybu.4.1697759728085; Thu, 19 Oct 2023 16:55:28 -0700 (PDT) Date: Thu, 19 Oct 2023 16:55:26 -0700 In-Reply-To: <20230911114347.85882-6-cloudliang@tencent.com> Mime-Version: 1.0 References: <20230911114347.85882-1-cloudliang@tencent.com> <20230911114347.85882-6-cloudliang@tencent.com> Message-ID: Subject: Re: [PATCH v4 5/9] KVM: selftests: Test Intel PMU architectural events on fixed counters From: Sean Christopherson To: Jinrong Liang Cc: Paolo Bonzini , Like Xu , David Matlack , Aaron Lewis , Vitaly Kuznetsov , Wanpeng Li , Jinrong Liang , kvm@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 11, 2023, Jinrong Liang wrote: > From: Jinrong Liang > > Update test to cover Intel PMU architectural events on fixed counters. > Per Intel SDM, PMU users can also count architecture performance events > on fixed counters (specifically, FIXED_CTR0 for the retired instructions > and FIXED_CTR1 for cpu core cycles event). Therefore, if guest's CPUID > indicates that an architecture event is not available, the corresponding > fixed counter will also not count that event. > > Co-developed-by: Like Xu > Signed-off-by: Like Xu > Signed-off-by: Jinrong Liang > --- > .../selftests/kvm/x86_64/pmu_counters_test.c | 22 +++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c > index f47853f3ab84..fe9f38a3557e 100644 > --- a/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c > +++ b/tools/testing/selftests/kvm/x86_64/pmu_counters_test.c > @@ -106,6 +106,28 @@ static void guest_measure_loop(uint8_t idx) > GUEST_ASSERT_EQ(expect, !!_rdpmc(i)); > } > > + if (this_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS) < 1) The ENTIRE point of reworking this_pmu_has() to be able to query fixed counters was to avoid having to open code checks like this. And this has no basis in reality, the fixed counters aren't all-or-nothing, e.g. if the number of fixed counters is '1', then the below will explode because the test will try to write a non-existent MSR. > + goto done; > + > + if (idx == INTEL_ARCH_INSTRUCTIONS_RETIRED) > + i = 0; > + else if (idx == INTEL_ARCH_CPU_CYCLES) > + i = 1; > + else if (idx == PSEUDO_ARCH_REFERENCE_CYCLES) > + i = 2; > + else > + goto done; > + > + wrmsr(MSR_CORE_PERF_FIXED_CTR0 + i, 0); > + wrmsr(MSR_CORE_PERF_FIXED_CTR_CTRL, BIT_ULL(4 * i)); > + > + wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, BIT_ULL(PMC_IDX_FIXED + i)); > + __asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES})); > + wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, 0); > + > + GUEST_ASSERT_EQ(expect, !!_rdpmc(PMC_FIXED_RDPMC_BASE | i)); Pulling in context from the previous patch, "expect" is set based on the existence of the architectural event in CPUID.0xA.EBX. That is completely bogus, especially for TSC reference cycles which has been incorrectly aliased to Topdown Slots. expect = this_pmu_has_arch_event(KVM_X86_PMU_FEATURE(UNUSED, idx)); Nothing in the SDM says that an event that's marked unavailable in CPUID.0xA.EBX magically makes the fixed counter not work. It's a rather bogus CPUID, e.g. I can't imagine real hardware has any such setup, but silently not counting is most definitely not correct. Digging around in KVM, I see that KVM _deliberately_ provides this behavior. That is just bogus. If the guest can enable a fixed counter, then it should count. *If* the interpretation of the SDM is that the fixed counter isn't available when the associated architectural event isn't available, then the most sane behavior is to not allow the fixed counter to be enabled in the first place. Silently doing nothing is awful. And again the whole Topdown Slots vs. TSC ref cycles confusion means this is completely broken regardless of how one interprets the SDM. And the above on-demand creation of each KVM_X86_PMU_FEATURE() kinda defeats the purpose of using well-known names. Rather than smush everything into the general purpose architectural events, which is definitely broken and arguably a straight violation of the SDM, I think the best option is to loosely couple the GP vs. fixed events so that we can reason about the high-level event type, e.g. to determine which events are "stable" enough to assert on. It's not the prettiest due to not being able to directly compare structs, but it at least allows checking for architectural events vs. fixed counters independently, without completely losing the common bits. #define X86_PMU_FEATURE_NULL \ ({ \ struct kvm_x86_pmu_feature feature = {}; \ \ feature; \ }) static bool pmu_is_null_feature(struct kvm_x86_pmu_feature event) { return !(*(u64 *)&event); } static void guest_measure_loop(uint8_t idx) { const struct { struct kvm_x86_pmu_feature gp_event; struct kvm_x86_pmu_feature fixed_event; } intel_event_to_feature[] = { [INTEL_ARCH_CPU_CYCLES] = { X86_PMU_FEATURE_CPU_CYCLES, X86_PMU_FEATURE_CPU_CYCLES_FIXED }, [INTEL_ARCH_INSTRUCTIONS_RETIRED] = { X86_PMU_FEATURE_INSNS_RETIRED, X86_PMU_FEATURE_INSNS_RETIRED_FIXED }, /* * Note, the fixed counter for reference cycles is NOT the same * as the general purpose architectural event (because the GP * event is garbage). The fixed counter explicitly counts at * the same frequency as the TSC, whereas the GP event counts * at a fixed, but uarch specific, frequency. Bundle them here * for simplicity. */ [INTEL_ARCH_REFERENCE_CYCLES] = { X86_PMU_FEATURE_REFERENCE_CYCLES, X86_PMU_FEATURE_REFERENCE_CYCLES_FIXED }, [INTEL_ARCH_LLC_REFERENCES] = { X86_PMU_FEATURE_LLC_REFERENCES, X86_PMU_FEATURE_NULL }, [INTEL_ARCH_LLC_MISSES] = { X86_PMU_FEATURE_LLC_MISSES, X86_PMU_FEATURE_NULL }, [INTEL_ARCH_BRANCHES_RETIRED] = { X86_PMU_FEATURE_BRANCH_INSNS_RETIRED, X86_PMU_FEATURE_NULL }, [INTEL_ARCH_BRANCHES_MISPREDICTED] = { X86_PMU_FEATURE_BRANCHES_MISPREDICTED, X86_PMU_FEATURE_NULL }, };