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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 27 Jul 2022 22:42:10 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Like Xu <like.xu.linux@...il.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [kvm-unit-tests PATCH] x86: Add tests for Guest Processor Event
 Based Sampling (PEBS)

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();
        }