[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <37b3bb47-059e-4524-a7d8-1aa28099cc94@linux.intel.com>
Date: Wed, 6 Mar 2024 15:55:11 +0800
From: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Kan Liang
<kan.liang@...ux.intel.com>, Like Xu <likexu@...cent.com>,
kvm@...r.kernel.org, linux-perf-users@...r.kernel.org,
linux-kernel@...r.kernel.org, Zhenyu Wang <zhenyuw@...ux.intel.com>,
Zhang Xiong <xiong.y.zhang@...el.com>, Lv Zhiyuan <zhiyuan.lv@...el.com>,
Dapeng Mi <dapeng1.mi@...el.com>, Mingwei Zhang <mizhang@...gle.com>
Subject: Re: [Patch v3] KVM: x86/pmu: Manipulate FIXED_CTR_CTRL MSR with
macros
On 3/6/2024 7:22 AM, Sean Christopherson wrote:
> +Mingwei
>
> On Thu, Aug 24, 2023, Dapeng Mi wrote:
> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
>> index 7d9ba301c090..ffda2ecc3a22 100644
>> --- a/arch/x86/kvm/pmu.h
>> +++ b/arch/x86/kvm/pmu.h
>> @@ -12,7 +12,8 @@
>> MSR_IA32_MISC_ENABLE_BTS_UNAVAIL)
>>
>> /* retrieve the 4 bits for EN and PMI out of IA32_FIXED_CTR_CTRL */
>> -#define fixed_ctrl_field(ctrl_reg, idx) (((ctrl_reg) >> ((idx)*4)) & 0xf)
>> +#define fixed_ctrl_field(ctrl_reg, idx) \
>> + (((ctrl_reg) >> ((idx) * INTEL_FIXED_BITS_STRIDE)) & INTEL_FIXED_BITS_MASK)
>>
>> #define VMWARE_BACKDOOR_PMC_HOST_TSC 0x10000
>> #define VMWARE_BACKDOOR_PMC_REAL_TIME 0x10001
>> @@ -165,7 +166,8 @@ static inline bool pmc_speculative_in_use(struct kvm_pmc *pmc)
>>
>> if (pmc_is_fixed(pmc))
>> return fixed_ctrl_field(pmu->fixed_ctr_ctrl,
>> - pmc->idx - INTEL_PMC_IDX_FIXED) & 0x3;
>> + pmc->idx - INTEL_PMC_IDX_FIXED) &
>> + (INTEL_FIXED_0_KERNEL | INTEL_FIXED_0_USER);
>>
>> return pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE;
>> }
>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
>> index f2efa0bf7ae8..b0ac55891cb7 100644
>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>> @@ -548,8 +548,13 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>> setup_fixed_pmc_eventsel(pmu);
>> }
>>
>> - for (i = 0; i < pmu->nr_arch_fixed_counters; i++)
>> - pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
>> + for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
>> + pmu->fixed_ctr_ctrl_mask &=
>> + ~intel_fixed_bits_by_idx(i,
>> + INTEL_FIXED_0_KERNEL |
>> + INTEL_FIXED_0_USER |
>> + INTEL_FIXED_0_ENABLE_PMI);
>> + }
>> counter_mask = ~(((1ull << pmu->nr_arch_gp_counters) - 1) |
>> (((1ull << pmu->nr_arch_fixed_counters) - 1) << INTEL_PMC_IDX_FIXED));
>> pmu->global_ctrl_mask = counter_mask;
>> @@ -595,7 +600,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
>> pmu->reserved_bits &= ~ICL_EVENTSEL_ADAPTIVE;
>> for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
>> pmu->fixed_ctr_ctrl_mask &=
>> - ~(1ULL << (INTEL_PMC_IDX_FIXED + i * 4));
> OMG, this might just win the award for most obfuscated PMU code in KVM, which is
> saying something. The fact that INTEL_PMC_IDX_FIXED happens to be 32, the same
> bit number as ICL_FIXED_0_ADAPTIVE, is 100% coincidence. Good riddance.
>
> Argh, and this goofy code helped introduce a real bug. reprogram_fixed_counters()
> doesn't account for the upper 32 bits of IA32_FIXED_CTR_CTRL.
>
> Wait, WTF? Nothing in KVM accounts for the upper bits. This can't possibly work.
>
> IIUC, because KVM _always_ sets precise_ip to a non-zero bit for PEBS events,
> perf will _always_ generate an adaptive record, even if the guest requested a
> basic record. Ugh, and KVM will always generate adaptive records even if the
> guest doesn't support them. This is all completely broken. It probably kinda
> sorta works because the Basic info is always stored in the record, and generating
> more info requires a non-zero MSR_PEBS_DATA_CFG, but ugh.
>
> Oh great, and it gets worse. intel_pmu_disable_fixed() doesn't clear the upper
> bits either, i.e. leaves ICL_FIXED_0_ADAPTIVE set. Unless I'm misreading the code,
> intel_pmu_enable_fixed() effectively doesn't clear ICL_FIXED_0_ADAPTIVE either,
> as it only modifies the bit when it wants to set ICL_FIXED_0_ADAPTIVE.
Currently the host PMU driver would always set the "Adaptive_Record" bit
in PERFEVTSELx and FIXED_CNTR_CTR MSRs as long as HW supports the
adaptive PEBS feature (See details in helpers intel_pmu_pebs_enable()
and intel_pmu_enable_fixed()).
It looks perf system doesn't export a interface to enable/disable the
adaptive PEBS. I suppose that's why KVM doesn't handle the
"Adaptive_Record" bit in ERFEVTSELx and FIXED_CNTR_CTR MSRs.
>
> *sigh*
>
> I'm _very_ tempted to disable KVM PEBS support for the current PMU, and make it
> available only when the so-called passthrough PMU is available[*]. Because I
> don't see how this is can possibly be functionally correct, nor do I see a way
> to make it functionally correct without a rather large and invasive series.
>
> Ouch. And after chatting with Mingwei, who asked the very good question of
> "can this leak host state?", I am pretty sure that yes, this can leak host state.
>
> When PERF_CAP_PEBS_BASELINE is enabled for the guest, i.e. when the guest has
> access to adaptive records, KVM gives the guest full access to MSR_PEBS_DATA_CFG
>
> pmu->pebs_data_cfg_mask = ~0xff00000full;
>
> which makes sense in a vacuum, because AFAICT the architecture doesn't allow
> exposing a subset of the four adaptive controls.
>
> GPRs and XMMs are always context switched and thus benign, but IIUC, Memory Info
> provides data that might now otherwise be available to the guest, e.g. if host
> userspace has disallowed equivalent events via KVM_SET_PMU_EVENT_FILTER.
>
> And unless I'm missing something, LBRs are a full leak of host state. Nothing
> in the SDM suggests that PEBS records honor MSR intercepts, so unless KVM is
> also passing through LBRs, i.e. is context switching all LBR MSRs, the guest can
> use PEBS to read host LBRs at will.
Not sure If I missed something, but I don't see there is leak of host
state. All perf events created by KVM would set "exclude_host"
attribute, that would leads to all guest counters including the PEBS
enabling counters would be disabled immediately by VMX once vm exits,
and so the PEBS engine would stop as well. I don't see a PEBS record
contains host state is possible to be written into guest DS area.
>
> Unless someone chimes in to point out how PEBS virtualization isn't a broken mess,
> I will post a patch to effectively disable PEBS virtualization.
>
> diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
> index 41a4533f9989..a2f827fa0ca1 100644
> --- a/arch/x86/kvm/vmx/capabilities.h
> +++ b/arch/x86/kvm/vmx/capabilities.h
> @@ -392,7 +392,7 @@ static inline bool vmx_pt_mode_is_host_guest(void)
>
> static inline bool vmx_pebs_supported(void)
> {
> - return boot_cpu_has(X86_FEATURE_PEBS) && kvm_pmu_cap.pebs_ept;
> + return false;
> }
>
> static inline bool cpu_has_notify_vmexit(void)
Powered by blists - more mailing lists