linux-kernel - Re: [PATCH V9 00/18] KVM: x86/pmu: Add *basic* support to enable guest PEBS via DS

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]

Message-ID: <YQF7lwM6qzYso0Gg@hirez.programming.kicks-ass.net>
Date:   Wed, 28 Jul 2021 17:45:27 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Zhu Lingshan <lingshan.zhu@...el.com>
Cc:     pbonzini@...hat.com, bp@...en8.de, seanjc@...gle.com,
        vkuznets@...hat.com, wanpengli@...cent.com, jmattson@...gle.com,
        joro@...tes.org, kan.liang@...ux.intel.com, ak@...ux.intel.com,
        wei.w.wang@...el.com, eranian@...gle.com, liuxiangdong5@...wei.com,
        linux-kernel@...r.kernel.org, x86@...nel.org, kvm@...r.kernel.org,
        like.xu.linux@...il.com, boris.ostrvsky@...cle.com
Subject: Re: [PATCH V9 00/18] KVM: x86/pmu: Add *basic* support to enable
 guest PEBS via DS

On Thu, Jul 22, 2021 at 01:41:41PM +0800, Zhu Lingshan wrote:
> The guest Precise Event Based Sampling (PEBS) feature can provide an
> architectural state of the instruction executed after the guest instruction
> that exactly caused the event. It needs new hardware facility only available
> on Intel Ice Lake Server platforms. This patch set enables the basic PEBS
> feature for KVM guests on ICX.
> 
> We can use PEBS feature on the Linux guest like native:
> 
>    # echo 0 > /proc/sys/kernel/watchdog (on the host)
>    # perf record -e instructions:ppp ./br_instr a
>    # perf record -c 100000 -e instructions:pp ./br_instr a

Why does the host need to disable the watchdog? IIRC ICL has multiple
PEBS capable counters. Also, I think the watchdog ends up on a fixed
counter by default anyway.

> Like Xu (17):
>   perf/core: Use static_call to optimize perf_guest_info_callbacks
>   perf/x86/intel: Add EPT-Friendly PEBS for Ice Lake Server
>   perf/x86/intel: Handle guest PEBS overflow PMI for KVM guest
>   perf/x86/core: Pass "struct kvm_pmu *" to determine the guest values
>   KVM: x86/pmu: Set MSR_IA32_MISC_ENABLE_EMON bit when vPMU is enabled
>   KVM: x86/pmu: Introduce the ctrl_mask value for fixed counter
>   KVM: x86/pmu: Add IA32_PEBS_ENABLE MSR emulation for extended PEBS
>   KVM: x86/pmu: Reprogram PEBS event to emulate guest PEBS counter
>   KVM: x86/pmu: Adjust precise_ip to emulate Ice Lake guest PDIR counter
>   KVM: x86/pmu: Add IA32_DS_AREA MSR emulation to support guest DS
>   KVM: x86/pmu: Add PEBS_DATA_CFG MSR emulation to support adaptive PEBS
>   KVM: x86: Set PEBS_UNAVAIL in IA32_MISC_ENABLE when PEBS is enabled
>   KVM: x86/pmu: Move pmc_speculative_in_use() to arch/x86/kvm/pmu.h
>   KVM: x86/pmu: Disable guest PEBS temporarily in two rare situations
>   KVM: x86/pmu: Add kvm_pmu_cap to optimize perf_get_x86_pmu_capability
>   KVM: x86/cpuid: Refactor host/guest CPU model consistency check
>   KVM: x86/pmu: Expose CPUIDs feature bits PDCM, DS, DTES64
> 
> Peter Zijlstra (Intel) (1):
>   x86/perf/core: Add pebs_capable to store valid PEBS_COUNTER_MASK value

Looks good:

Acked-by: Peter Zijlstra (Intel) <peterz@...radead.org>

How do we want to route this, all through the KVM tree?

One little nit I had; would something like the below (on top perhaps)
make the code easier to read?

---
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -3921,9 +3921,12 @@ static struct perf_guest_switch_msr *int
 	struct kvm_pmu *kvm_pmu = (struct kvm_pmu *)data;
 	u64 intel_ctrl = hybrid(cpuc->pmu, intel_ctrl);
 	u64 pebs_mask = cpuc->pebs_enabled & x86_pmu.pebs_capable;
+	int global_ctrl, pebs_enable;
 
 	*nr = 0;
-	arr[(*nr)++] = (struct perf_guest_switch_msr){
+
+	global_ctrl = (*nr)++;
+	arr[global_ctrl] = (struct perf_guest_switch_msr){
 		.msr = MSR_CORE_PERF_GLOBAL_CTRL,
 		.host = intel_ctrl & ~cpuc->intel_ctrl_guest_mask,
 		.guest = intel_ctrl & (~cpuc->intel_ctrl_host_mask | ~pebs_mask),
@@ -3966,23 +3969,23 @@ static struct perf_guest_switch_msr *int
 		};
 	}
 
-	arr[*nr] = (struct perf_guest_switch_msr){
+	pebs_enable = (*nr)++;
+	arr[pebs_enable] = (struct perf_guest_switch_msr){
 		.msr = MSR_IA32_PEBS_ENABLE,
 		.host = cpuc->pebs_enabled & ~cpuc->intel_ctrl_guest_mask,
 		.guest = pebs_mask & ~cpuc->intel_ctrl_host_mask,
 	};
 
-	if (arr[*nr].host) {
+	if (arr[pebs_enable].host) {
 		/* Disable guest PEBS if host PEBS is enabled. */
-		arr[*nr].guest = 0;
+		arr[pebs_enable].guest = 0;
 	} else {
 		/* Disable guest PEBS for cross-mapped PEBS counters. */
-		arr[*nr].guest &= ~kvm_pmu->host_cross_mapped_mask;
+		arr[pebs_enable].guest &= ~kvm_pmu->host_cross_mapped_mask;
 		/* Set hw GLOBAL_CTRL bits for PEBS counter when it runs for guest */
-		arr[0].guest |= arr[*nr].guest;
+		arr[global_ctrl].guest |= arr[pebs_enable].guest;
 	}
 
-	++(*nr);
 	return arr;
 }