| 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
| ||
|
Message-ID: <20200409163717.GD20713@hirez.programming.kicks-ass.net>
Date: Thu, 9 Apr 2020 18:37:17 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Like Xu <like.xu@...ux.intel.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
Andi Kleen <ak@...ux.intel.com>,
Jim Mattson <jmattson@...gle.com>,
Wanpeng Li <wanpengli@...cent.com>,
Sean Christopherson <sean.j.christopherson@...el.com>,
Joerg Roedel <joro@...tes.org>,
Liran Alon <liran.alon@...cle.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Liang Kan <kan.liang@...ux.intel.com>,
Wei Wang <wei.w.wang@...el.com>, linux-kernel@...r.kernel.org,
Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH v9 03/10] perf/x86: Add constraint to create guest LBR
event without hw counter
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 3bb738f5a472..e919187a0751 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -74,7 +74,8 @@ u64 x86_perf_event_update(struct perf_event *event)
> int idx = hwc->idx;
> u64 delta;
>
> - if (idx == INTEL_PMC_IDX_FIXED_BTS)
> + if ((idx == INTEL_PMC_IDX_FIXED_BTS) ||
> + (idx == INTEL_PMC_IDX_FIXED_VLBR))
> return 0;
>
> /*
> @@ -1102,7 +1103,8 @@ static inline void x86_assign_hw_event(struct perf_event *event,
> hwc->last_cpu = smp_processor_id();
> hwc->last_tag = ++cpuc->tags[i];
>
> - if (hwc->idx == INTEL_PMC_IDX_FIXED_BTS) {
> + if ((hwc->idx == INTEL_PMC_IDX_FIXED_BTS) ||
> + (hwc->idx == INTEL_PMC_IDX_FIXED_VLBR)) {
> hwc->config_base = 0;
> hwc->event_base = 0;
> } else if (hwc->idx >= INTEL_PMC_IDX_FIXED) {
> @@ -1233,7 +1235,8 @@ int x86_perf_event_set_period(struct perf_event *event)
> s64 period = hwc->sample_period;
> int ret = 0, idx = hwc->idx;
>
> - if (idx == INTEL_PMC_IDX_FIXED_BTS)
> + if ((idx == INTEL_PMC_IDX_FIXED_BTS) ||
> + (idx == INTEL_PMC_IDX_FIXED_VLBR))
> return 0;
>
> /*
That seems unfortunate; can that be >= INTEL_PMC_IDX_FIXED_BTS ? If so,
that probably wants a comment with the definitions.
Or otherwise check for !hwc->event_base. That should be 0 for both these
things.
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 3be51aa06e67..901c82032f4a 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -2157,6 +2157,9 @@ static void intel_pmu_disable_event(struct perf_event *event)
> return;
> }
>
> + if (unlikely(hwc->idx == INTEL_PMC_IDX_FIXED_VLBR))
> + return;
> +
Please check code-gen to see if you can cut down on brancher here;
there's 4 cases:
- vlbr
- bts
- fixed
- gp
perhaps you can write it like so:
(also see https://lkml.kernel.org/r/20190828090217.GN2386@hirez.programming.kicks-ass.net )
static void intel_pmu_enable_event(struct perf_event *event)
{
...
int idx = hwx->idx;
if (idx < INTEL_PMC_IDX_FIXED) {
intel_set_masks(event, idx);
__x86_pmu_enable_event(hwc, ARCH_PERFMON_EVENTSEL_ENABLE);
} else if (idx < INTEL_PMC_IDX_FIXED_BTS) {
intel_set_masks(event, idx);
intel_pmu_enable_fixed(event);
} else if (idx == INTEL_PMC_IDX_FIXED_BTS) {
intel_pmu_enable_bts(hwc->config);
}
/* nothing for INTEL_PMC_IDX_FIXED_VLBR */
}
That should sort the branches in order of: gp,fixed,bts,vlbr
> cpuc->intel_ctrl_guest_mask &= ~(1ull << hwc->idx);
> cpuc->intel_ctrl_host_mask &= ~(1ull << hwc->idx);
> cpuc->intel_cp_status &= ~(1ull << hwc->idx);
> @@ -2241,6 +2244,9 @@ static void intel_pmu_enable_event(struct perf_event *event)
> return;
> }
>
> + if (unlikely(hwc->idx == INTEL_PMC_IDX_FIXED_VLBR))
> + return;
> +
> if (event->attr.exclude_host)
> cpuc->intel_ctrl_guest_mask |= (1ull << hwc->idx);
> if (event->attr.exclude_guest)
idem.
> @@ -2595,6 +2601,15 @@ intel_bts_constraints(struct perf_event *event)
> return NULL;
> }
>
> +static struct event_constraint *
> +intel_guest_event_constraints(struct perf_event *event)
> +{
> + if (unlikely(is_guest_lbr_event(event)))
> + return &guest_lbr_constraint;
> +
> + return NULL;
> +}
This is a mis-nomer, it isn't just any guest_event
> +
> static int intel_alt_er(int idx, u64 config)
> {
> int alt_idx = idx;
> @@ -2785,6 +2800,10 @@ __intel_get_event_constraints(struct cpu_hw_events *cpuc, int idx,
> {
> struct event_constraint *c;
>
> + c = intel_guest_event_constraints(event);
> + if (c)
> + return c;
> +
> c = intel_bts_constraints(event);
> if (c)
> return c;
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index 1025bc6eb04f..9a62264a3068 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -969,6 +969,20 @@ static inline bool intel_pmu_has_bts(struct perf_event *event)
> return intel_pmu_has_bts_period(event, hwc->sample_period);
> }
>
> +static inline bool is_guest_event(struct perf_event *event)
> +{
> + if (event->attr.exclude_host && is_kernel_event(event))
> + return true;
> + return false;
> +}
I don't like this one, what if another in-kernel users generates an
event with exclude_host set ?
> @@ -989,6 +1003,7 @@ void release_ds_buffers(void);
> void reserve_ds_buffers(void);
>
> extern struct event_constraint bts_constraint;
> +extern struct event_constraint guest_lbr_constraint;
>
> void intel_pmu_enable_bts(u64 config);
>
> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
> index e018a1cf604c..674130aca75a 100644
> --- a/arch/x86/include/asm/perf_event.h
> +++ b/arch/x86/include/asm/perf_event.h
> @@ -181,9 +181,19 @@ struct x86_pmu_capability {
> #define GLOBAL_STATUS_UNC_OVF BIT_ULL(61)
> #define GLOBAL_STATUS_ASIF BIT_ULL(60)
> #define GLOBAL_STATUS_COUNTERS_FROZEN BIT_ULL(59)
> -#define GLOBAL_STATUS_LBRS_FROZEN BIT_ULL(58)
> +#define GLOBAL_STATUS_LBRS_FROZEN_BIT 58
> +#define GLOBAL_STATUS_LBRS_FROZEN BIT_ULL(GLOBAL_STATUS_LBRS_FROZEN_BIT)
> #define GLOBAL_STATUS_TRACE_TOPAPMI BIT_ULL(55)
>
> +/*
> + * We model guest LBR event tracing as another fixed-mode PMC like BTS.
> + *
> + * We choose bit 58 (LBRS_FROZEN_BIT) which is used to indicate that the LBR
> + * stack is frozen on a hardware PMI request in the PERF_GLOBAL_STATUS msr,
> + * and the 59th PMC counter (if any) is not supposed to use it as well.
Is this saying that STATUS.58 should never be set? I don't really
understand the language.
> + */
> +#define INTEL_PMC_IDX_FIXED_VLBR GLOBAL_STATUS_LBRS_FROZEN_BIT
> +
> /*
> * Adaptive PEBS v4
> */
Powered by blists - more mailing lists