linux-kernel - Re: [PATCH v11 05/11] perf/x86: Keep LBR stack unchanged in host context for guest LBR event

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:   Tue, 19 May 2020 21:25:19 +0800
From:   "Xu, Like" <like.xu@...el.com>
To:     Peter Zijlstra <peterz@...radead.org>,
        Like Xu <like.xu@...ux.intel.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>, ak@...ux.intel.com,
        wei.w.wang@...el.com
Subject: Re: [PATCH v11 05/11] perf/x86: Keep LBR stack unchanged in host
 context for guest LBR event

Hi Peter,

On 2020/5/19 18:45, Peter Zijlstra wrote:
> On Tue, May 19, 2020 at 11:08:41AM +0800, Like Xu wrote:
>
>> Sure, I could reuse cpuc->intel_ctrl_guest_mask to rewrite this part:
>>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index d788edb7c1f9..f1243e8211ca 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
>> @@ -2189,7 +2189,8 @@ static void intel_pmu_disable_event(struct perf_event
>> *event)
>>          } else if (idx == INTEL_PMC_IDX_FIXED_BTS) {
>>                  intel_pmu_disable_bts();
>>                  intel_pmu_drain_bts_buffer();
>> -       }
>> +       } else if (idx == INTEL_PMC_IDX_FIXED_VLBR)
>> +               intel_clear_masks(event, idx);
>>
>>          /*
>>           * Needs to be called after x86_pmu_disable_event,
>> @@ -2271,7 +2272,8 @@ static void intel_pmu_enable_event(struct perf_event
>> *event)
>>                  if (!__this_cpu_read(cpu_hw_events.enabled))
>>                          return;
>>                  intel_pmu_enable_bts(hwc->config);
>> -       }
>> +       } else if (idx == INTEL_PMC_IDX_FIXED_VLBR)
>> +               intel_set_masks(event, idx);
>>   }
> This makes me wonder if we can pull intel_{set,clear}_masks() out of
> that if()-forest, but that's something for later...
>
>>   static void intel_pmu_add_event(struct perf_event *event)
>> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
>> index b8dabf1698d6..1b30c76815dd 100644
>> --- a/arch/x86/events/intel/lbr.c
>> +++ b/arch/x86/events/intel/lbr.c
>> @@ -552,11 +552,19 @@ void intel_pmu_lbr_del(struct perf_event *event)
>>          perf_sched_cb_dec(event->ctx->pmu);
>>   }
>>
>> +static inline bool vlbr_is_enabled(void)
>> +{
>> +       struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>> +
>> +       return test_bit(INTEL_PMC_IDX_FIXED_VLBR,
>> +               (unsigned long *)&cpuc->intel_ctrl_guest_mask);
>> +}
> Maybe call this: vlbr_exclude_host() ?
Sure, I'll apply it.
>
>> +
>>   void intel_pmu_lbr_enable_all(bool pmi)
>>   {
>>          struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>>
>> -       if (cpuc->lbr_users)
>> +       if (cpuc->lbr_users && !vlbr_is_enabled())
>>                  __intel_pmu_lbr_enable(pmi);
>>   }
>>
>> @@ -564,7 +572,7 @@ void intel_pmu_lbr_disable_all(void)
>>   {
>>          struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>>
>> -       if (cpuc->lbr_users)
>> +       if (cpuc->lbr_users && !vlbr_is_enabled())
>>                  __intel_pmu_lbr_disable();
>>   }
>>
>> @@ -706,7 +714,8 @@ void intel_pmu_lbr_read(void)
>>           * This could be smarter and actually check the event,
>>           * but this simple approach seems to work for now.
>>           */
>> -       if (!cpuc->lbr_users || cpuc->lbr_users == cpuc->lbr_pebs_users)
>> +       if (!cpuc->lbr_users || vlbr_is_enabled() ||
>> +               cpuc->lbr_users == cpuc->lbr_pebs_users)
>>                  return;
>>
>>          if (x86_pmu.intel_cap.lbr_format == LBR_FORMAT_32)
>>
>> Is this acceptable to you ?
> Yeah, looks about right. Let me stare at the rest.
Uh, thanks for your warmly comments on the rest KVM part.

Let me assume we do not have any blocking issues
on the host perf changes to enable LBR feature for guests.

Thanks,
Like Xu