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;
Message-ID: <7efeb037-e82c-8401-2e7a-a880f3f21d9c@gmail.com>
Date:   Thu, 28 Jul 2022 19:31:49 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.11.0
Subject: Re: [kvm-unit-tests PATCH] x86: Add tests for Guest Processor Event
 Based Sampling (PEBS)
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Jim Mattson <jmattson@google.com>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
References: <20220721103549.49543-1-likexu@tencent.com>
 <20220721103549.49543-9-likexu@tencent.com> <YuG/QtIM/fvhLI/u@google.com>
From:   Like Xu <like.xu.linux@gmail.com>
In-Reply-To: <YuG/QtIM/fvhLI/u@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 28/7/2022 6:42 am, Sean Christopherson wrote:
> On Thu, Jul 21, 2022, Like Xu wrote:
>> +union perf_capabilities {
>> +	struct {
>> +		u64	lbr_format:6;
>> +		u64	pebs_trap:1;
>> +		u64	pebs_arch_reg:1;
>> +		u64	pebs_format:4;
>> +		u64	smm_freeze:1;
>> +		/*
>> +		 * PMU supports separate counter range for writing
>> +		 * values > 32bit.
>> +		 */
>> +		u64	full_width_write:1;
>> +		u64 pebs_baseline:1;
>> +		u64	perf_metrics:1;
>> +		u64	pebs_output_pt_available:1;
>> +		u64	anythread_deprecated:1;
>> +	};
>> +	u64	capabilities;
>> +};
>> +
>> +union cpuid10_eax {
>> +        struct {
>> +                unsigned int version_id:8;
>> +                unsigned int num_counters:8;
>> +                unsigned int bit_width:8;
>> +                unsigned int mask_length:8;
>> +        } split;
>> +        unsigned int full;
>> +} pmu_eax;
>> +
>> +union cpuid10_edx {
>> +        struct {
>> +                unsigned int num_counters_fixed:5;
>> +                unsigned int bit_width_fixed:8;
>> +                unsigned int reserved:19;
>> +        } split;
>> +        unsigned int full;
>> +} pmu_edx;
> 
> The generic unions are hopefully unnecessary now that helpers are provided by
> lib/x86/processor.h, e.g. for pmu_version().
> 
> I would prefer to have similar helpers instead of "union perf_capabilities",
> but it's not a sticking point if helpers a signifiantly more painful to use.
> 
>> +	if (!is_intel() || (pmu_eax.split.version_id < 2) ||
>> +	    !(perf.capabilities & PERF_CAP_PEBS_FORMAT) ||
>> +	    (rdmsr(MSR_IA32_MISC_ENABLE) & MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL)) {
> 
> Split these up, it's really, really annoying to have to guess which one of the
> four checks failed.
> 
>> +		report_skip("This platform doesn't support guest PEBS.");
>> +		return 0;
> 
> This needs be be "return report_summary()", otherwise the test says pass when it
> didn't do anyting:
> 
>   TESTNAME=pmu_pebs TIMEOUT=90s ACCEL=kvm ./x86/run x86/pmu_pebs.flat -smp 1 -cpu host,migratable=no
>   PASS pmu_pebs
> 
> wait a second...
> 
>    SKIP: This platform doesn't support guest PEBS.
> 
> E.g. (though if KUT can provide more information on why PERF_CAP_PEBS_FORMAT
> may not be advertised, e.g. requires ICX+?, that would be nice to have)
> 
>          if (!is_intel()) {
>                  report_skip("PEBS is only supported on Intel CPUs");
>                  return report_summary();
>          }
>          if (pmu_version() < 2) {
>                  report_skip("Architectural PMU not available");
>                  return report_summary();
>          }
>          if (!(perf.capabilities & PERF_CAP_PEBS_FORMAT)) {
>                  report_skip("PEBS not enumerated in PERF_CAPABILITIES");
>                  return report_summary();
>          }
>          if (rdmsr(MSR_IA32_MISC_ENABLE) & MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL) {
>                  report_skip("PEBS unavailable according to MISC_ENABLE");
>                  return report_summary();
>          }

Thanks, all applied. Please review this new version as a separate thread.
https://lore.kernel.org/kvm/20220728112119.58173-1-likexu@tencent.com/