Re: [kvm-unit-tests PATCH] x86: Add tests for Guest Processor Event Based Sampling (PEBS)

Like Xu
2022-07-28T11:32:02Z

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.