Re: [PATCH 1/2] perf: ignore LBR and offcore_rsp.

Re: [PATCH 1/2] perf: ignore LBR and offcore_rsp.

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]

On Wed, Jun 18, 2014 at 03:52:55PM +0000, Liang, Kan wrote:
>     perf ignore LBR and offcore_rsp.
> 
>     x86, perf: Protect LBR and offcore rsp against KVM lying
> 
>     With -cpu host, KVM reports LBR and offcore support, if the host has support.
>     When the guest perf driver tries to access LBR or offcore_rsp MSR,
>     it #GPs all MSR accesses,since KVM doesn't handle LBR and offcore support.
>     So LBR and offcore rsp MSRs are needed to be protected by _safe().
> 
>     For reproducing the issue, please build the kernel with CONFIG_KVM_INTEL = y.
>     And CONFIG_PARAVIRT = n and CONFIG_KVM_GUEST = n.
>     Start the guest with -cpu host.
>     Run perf record with --branch-any or --branch-filter in guest to trigger LBR #GP.
>     Run perf stat offcore events (E.g. LLC-loads/LLC-load-misses ...) in guest to trigger offcore_rsp #GP
> 
>     Signed-off-by: Andi Kleen <ak@linux.intel.com>

This order indicates Andi is the author; but there's no corresponding
From.

>     Signed-off-by: Kan Liang <kan.liang@intel.com>

And here I thought that Andi was of the opinion that if you set CPUID to
indicate a particular CPU you had better also handle all its MSRs.



[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] perf: ignore LBR and offcore_rsp.

[Andi Kleen]

Peter Zijlstra <peterz@infradead.org> writes:
>
> This order indicates Andi is the author; but there's no corresponding
> From.

I wrote an early version of the patch, but Kan took it over and extended
it. So both are authors.

BTW Kan you may want to use git send-email to get standard format.

>
>>     Signed-off-by: Kan Liang <kan.liang@intel.com>
>
> And here I thought that Andi was of the opinion that if you set CPUID to
> indicate a particular CPU you had better also handle all its MSRs.

Yes, philosophically that would be the right way,
but we needed a short term fix to stop things from crashing, and that
was the simplest.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 1/2] perf: ignore LBR and offcore_rsp.

[Andi Kleen]

Andi Kleen <andi@firstfloor.org> writes:
>>
>>>     Signed-off-by: Kan Liang <kan.liang@intel.com>
>>
>> And here I thought that Andi was of the opinion that if you set CPUID to
>> indicate a particular CPU you had better also handle all its MSRs.
>
> Yes, philosophically that would be the right way,
> but we needed a short term fix to stop things from crashing, and that
> was the simplest.

I should add there is another reason for this patch now,
and doing it in perf instead of somewhere else
(this should probably go into the description).

With PT on enabling LBR can #GP. So perf needs to handle
this case without crashing. This can happen independently
of any hypervisors.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 1/2] perf: ignore LBR and offcore_rsp.

[Peter Zijlstra]

On Thu, Jun 19, 2014 at 10:52:52AM -0700, Andi Kleen wrote:
> Andi Kleen <andi@firstfloor.org> writes:
> >>
> >>>     Signed-off-by: Kan Liang <kan.liang@intel.com>
> >>
> >> And here I thought that Andi was of the opinion that if you set CPUID to
> >> indicate a particular CPU you had better also handle all its MSRs.
> >
> > Yes, philosophically that would be the right way,
> > but we needed a short term fix to stop things from crashing, and that
> > was the simplest.
> 
> I should add there is another reason for this patch now,
> and doing it in perf instead of somewhere else
> (this should probably go into the description).
> 
> With PT on enabling LBR can #GP. So perf needs to handle
> this case without crashing. This can happen independently
> of any hypervisors.

WTH is a PT?

Re: [PATCH 1/2] perf: ignore LBR and offcore_rsp.

[Peter Zijlstra]

On Wed, Jun 18, 2014 at 03:52:55PM +0000, Liang, Kan wrote:
> diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
> index 3b2f9bd..f828ddd 100644
> --- a/arch/x86/kernel/cpu/perf_event.h
> +++ b/arch/x86/kernel/cpu/perf_event.h
> @@ -555,8 +555,9 @@ static inline void __x86_pmu_enable_event(struct hw_perf_event *hwc,
> {
>     u64 disable_mask = __this_cpu_read(cpu_hw_events.perf_ctr_virt_mask);
> -    if (hwc->extra_reg.reg)
> -          wrmsrl(hwc->extra_reg.reg, hwc->extra_reg.config);
> +    if (hwc->extra_reg.reg &&
> +          (wrmsrl_safe(hwc->extra_reg.reg, hwc->extra_reg.config) < 0))
> +          return;
>     wrmsrl(hwc->config_base, (hwc->config | enable_mask) & ~disable_mask);
> }
> diff --git a/arch/x86/kernel/cpu/perf_event_intel_lbr.c b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> index d82d155..6f2d1e9 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
> @@ -157,7 +157,8 @@ static void intel_pmu_lbr_reset_32(void)
>     int i;
>      for (i = 0; i < x86_pmu.lbr_nr; i++)
> -          wrmsrl(x86_pmu.lbr_from + i, 0);
> +          if (wrmsrl_safe(x86_pmu.lbr_from + i, 0ULL) < 0)
> +               return;
> }
>  static void intel_pmu_lbr_reset_64(void)
> @@ -165,8 +166,9 @@ static void intel_pmu_lbr_reset_64(void)
>     int i;
>      for (i = 0; i < x86_pmu.lbr_nr; i++) {
> -          wrmsrl(x86_pmu.lbr_from + i, 0);
> -          wrmsrl(x86_pmu.lbr_to   + i, 0);
> +          if (wrmsrl_safe(x86_pmu.lbr_from + i, 0ULL) < 0 ||
> +              wrmsrl_safe(x86_pmu.lbr_to   + i, 0ULL) < 0)
> +               return;
>     }
> }
> @@ -241,7 +243,7 @@ static inline u64 intel_pmu_lbr_tos(void)
> {
>     u64 tos;
> -    rdmsrl(x86_pmu.lbr_tos, tos);
> +    rdmsrl_safe(x86_pmu.lbr_tos, &tos);
>      return tos;
> }
> @@ -262,7 +264,9 @@ static void intel_pmu_lbr_read_32(struct cpu_hw_events *cpuc)
>                u64     lbr;
>           } msr_lastbranch;
> -          rdmsrl(x86_pmu.lbr_from + lbr_idx, msr_lastbranch.lbr);
> +          if (rdmsrl_safe(x86_pmu.lbr_from + lbr_idx,
> +                    &msr_lastbranch.lbr) < 0)
> +               break;
>            cpuc->lbr_entries[i].from  = msr_lastbranch.from;
>           cpuc->lbr_entries[i].to         = msr_lastbranch.to;
> @@ -292,8 +296,9 @@ static void intel_pmu_lbr_read_64(struct cpu_hw_events *cpuc)
>           int skip = 0;
>           int lbr_flags = lbr_desc[lbr_format];
> -          rdmsrl(x86_pmu.lbr_from + lbr_idx, from);
> -          rdmsrl(x86_pmu.lbr_to   + lbr_idx, to);
> +          if (rdmsrl_safe(x86_pmu.lbr_from + lbr_idx, &from) < 0 ||
> +              rdmsrl_safe(x86_pmu.lbr_to   + lbr_idx, &to) < 0)
> +               break;
>            if (lbr_flags & LBR_EIP_FLAGS) {
>                mis = !!(from & LBR_FROM_FLAG_MISPRED);

So I really hate this patch, it makes the code hideous. Also, its a
death by a thousand cuts adding endless branches in this code.

Also, the offcore part is retarded, just make sure extra_reg isn't set.

As to the LBR, just make it so that we avoid calling the LBR code to
begin with; ideally without adding extra code to fast paths.

Re: [PATCH 1/2] perf: ignore LBR and offcore_rsp.

[Andi Kleen]

Peter Zijlstra <peterz@infradead.org> writes:

>
> So I really hate this patch, it makes the code hideous. Also, its a
> death by a thousand cuts adding endless branches in this code.

FWIW compared to the cost of a RDMSR (which is a very complex operation
for the CPU) the cost of a predicted branch is nearly in the noise.

>
> Also, the offcore part is retarded, just make sure extra_reg isn't set.
>
> As to the LBR, just make it so that we avoid calling the LBR code to
> begin with; ideally without adding extra code to fast paths.

You mean check once at initialization time.

I'm not sure that would handle all cases unfortunately.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only