| Message ID | 20211210133525.46465-2-likexu@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v11,01/17] perf/x86/intel: Add EPT-Friendly PEBS for Ice Lake Server | expand |
On Fri, Dec 10, 2021, Like Xu wrote: > From: Like Xu <like.xu@linux.intel.com> > > From: Like Xu <like.xu@linux.intel.com> Did one of these get handcoded? > The new hardware facility supporting guest PEBS is only available on > Intel Ice Lake Server platforms for now. KVM will check this field > through perf_get_x86_pmu_capability() instead of hard coding the cpu > models in the KVM code. If it is supported, the guest PEBS capability > will be exposed to the guest. So what exactly is this new feature? I've speed read the cover letter and a few changelogs and didn't find anything that actually explained when this feature does. Based on the shortlog, I assume the feature handles translating linear addresses via EPT? If that's correct, then x86_pmu.pebs_vmx should be named something like x86_pmu.pebs_ept. That also raises the question of what will happen if EPT is disabled. Presumably things will Just Work since no additional translation is needed, but if that's the case then arguably vmx_pebs_supported() should be: return boot_cpu_has(X86_FEATURE_PEBS) && (!tdp_enabled || kvm_pmu_cap.pebs_vmx); I'm guessing no one actually cares about supporting PEBS on older CPUs using shadow paging, but the changelog should at least call out that PEBS is allowed if and only if "pebs_vmx" is supported for simplicity, even though it would actually work if EPT is disabled. And if for some reason it _doesn't_ work when EPT is disabled, then vmx_pebs_supported() and friends need to actually check tdp_enabled. Regardless, this changelog really, really needs an explanation of the feature.
On 31/12/2021 2:13 am, Sean Christopherson wrote: > On Fri, Dec 10, 2021, Like Xu wrote: >> From: Like Xu <like.xu@linux.intel.com> >> >> From: Like Xu <like.xu@linux.intel.com> > > Did one of these get handcoded? Uh, now I have found the use of "--from=<ident>". > >> The new hardware facility supporting guest PEBS is only available on >> Intel Ice Lake Server platforms for now. KVM will check this field >> through perf_get_x86_pmu_capability() instead of hard coding the cpu >> models in the KVM code. If it is supported, the guest PEBS capability >> will be exposed to the guest. > > So what exactly is this new feature? I've speed read the cover letter and a few > changelogs and didn't find anything that actually explained when this feature does. > Please check Intel SDM Vol3 18.9.5 for this "EPT-Friendly PEBS" feature. I assume when an unfamiliar feature appears in the patch SUBJECT, the reviewer may search for the exact name in the specification. > Based on the shortlog, I assume the feature handles translating linear addresses > via EPT? If that's correct, then x86_pmu.pebs_vmx should be named something like > x86_pmu.pebs_ept. "Translating linear addresses via EPT" is only part of the hardware implementation, and we may apply the new name if there are no other objections. > > That also raises the question of what will happen if EPT is disabled. Presumably > things will Just Work since no additional translation is needed, but if that's the > case then arguably vmx_pebs_supported() should be: > > return boot_cpu_has(X86_FEATURE_PEBS) && > (!tdp_enabled || kvm_pmu_cap.pebs_vmx); Yes, a similar fix is already on my private tree, and thank you for pointing it out! > > I'm guessing no one actually cares about supporting PEBS on older CPUs using shadow > paging, but the changelog should at least call out that PEBS is allowed if and only > if "pebs_vmx" is supported for simplicity, even though it would actually work if EPT > is disabled. And if for some reason it _doesn't_ work when EPT is disabled, then > vmx_pebs_supported() and friends need to actually check tdp_enabled. Yes, the guest PEBS only works when EPT is enabled on the newer modern CPUs. > > Regardless, this changelog really, really needs an explanation of the feature. Thank you for picking up, I will update the changelog for this commit. Please let me know if you have any more obstacles or niggles to review this patch set. Thanks, Like Xu
On Fri, Dec 31, 2021, Like Xu wrote: > On 31/12/2021 2:13 am, Sean Christopherson wrote: > > On Fri, Dec 10, 2021, Like Xu wrote: > > > The new hardware facility supporting guest PEBS is only available on > > > Intel Ice Lake Server platforms for now. KVM will check this field > > > through perf_get_x86_pmu_capability() instead of hard coding the cpu > > > models in the KVM code. If it is supported, the guest PEBS capability > > > will be exposed to the guest. > > > > So what exactly is this new feature? I've speed read the cover letter and a few > > changelogs and didn't find anything that actually explained when this feature does. > > > > Please check Intel SDM Vol3 18.9.5 for this "EPT-Friendly PEBS" feature. > > I assume when an unfamiliar feature appears in the patch SUBJECT, > the reviewer may search for the exact name in the specification. C'mon, seriously? How the blazes am I supposed to know that the feature name is EPT-Friendly PEBS? Or that it's even in the SDM (it's not in the year-old version of the SDM I currently have open) versus one of the many ISE docs? This is not hard. Please spend the 30 seconds it takes to write a small blurb so that reviewers don't have to spend 5+ minutes wondering WTF this does. Add support for EPT-Friendly PEBS, a new CPU feature that enlightens PEBS to translate guest linear address through EPT, and facilitates handling VM-Exits that occur when accessing PEBS records. More information can be found in the <date> release of Intel's SDM, Volume 3, 18.9.5 "EPT-Friendly PEBS".
On 5/1/2022 1:25 am, Sean Christopherson wrote: > On Fri, Dec 31, 2021, Like Xu wrote: >> On 31/12/2021 2:13 am, Sean Christopherson wrote: >>> On Fri, Dec 10, 2021, Like Xu wrote: >>>> The new hardware facility supporting guest PEBS is only available on >>>> Intel Ice Lake Server platforms for now. KVM will check this field >>>> through perf_get_x86_pmu_capability() instead of hard coding the cpu >>>> models in the KVM code. If it is supported, the guest PEBS capability >>>> will be exposed to the guest. >>> >>> So what exactly is this new feature? I've speed read the cover letter and a few >>> changelogs and didn't find anything that actually explained when this feature does. >>> >> >> Please check Intel SDM Vol3 18.9.5 for this "EPT-Friendly PEBS" feature. >> >> I assume when an unfamiliar feature appears in the patch SUBJECT, >> the reviewer may search for the exact name in the specification. > > C'mon, seriously? How the blazes am I supposed to know that the feature name > is EPT-Friendly PEBS? Or that it's even in the SDM (it's not in the year-old > version of the SDM I currently have open) versus one of the many ISE docs? You're right. The reviewer's time is valuable. Apologies for my wrong assumption. > > This is not hard. Please spend the 30 seconds it takes to write a small blurb > so that reviewers don't have to spend 5+ minutes wondering WTF this does. > > Add support for EPT-Friendly PEBS, a new CPU feature that enlightens PEBS to > translate guest linear address through EPT, and facilitates handling VM-Exits > that occur when accessing PEBS records. More information can be found in the > <date> release of Intel's SDM, Volume 3, 18.9.5 "EPT-Friendly PEBS". Applied and thanks.
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c index 38b2c779146f..03133e96ddb0 100644 --- a/arch/x86/events/core.c +++ b/arch/x86/events/core.c @@ -2993,5 +2993,6 @@ void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap) cap->bit_width_fixed = x86_pmu.cntval_bits; cap->events_mask = (unsigned int)x86_pmu.events_maskl; cap->events_mask_len = x86_pmu.events_mask_len; + cap->pebs_vmx = x86_pmu.pebs_vmx; } EXPORT_SYMBOL_GPL(perf_get_x86_pmu_capability); diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c index ec6444f2c9dc..869684ed55b1 100644 --- a/arch/x86/events/intel/core.c +++ b/arch/x86/events/intel/core.c @@ -6103,6 +6103,7 @@ __init int intel_pmu_init(void) case INTEL_FAM6_ICELAKE_X: case INTEL_FAM6_ICELAKE_D: + x86_pmu.pebs_vmx = 1; pmem = true; fallthrough; case INTEL_FAM6_ICELAKE_L: diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h index 5480db242083..fdda099867c2 100644 --- a/arch/x86/events/perf_event.h +++ b/arch/x86/events/perf_event.h @@ -814,7 +814,8 @@ struct x86_pmu { pebs_prec_dist :1, pebs_no_tlb :1, pebs_no_isolation :1, - pebs_block :1; + pebs_block :1, + pebs_vmx :1; int pebs_record_size; int pebs_buffer_size; int max_pebs_events; diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h index 8fc1b5003713..42d7bcf1a896 100644 --- a/arch/x86/include/asm/perf_event.h +++ b/arch/x86/include/asm/perf_event.h @@ -192,6 +192,7 @@ struct x86_pmu_capability { int bit_width_fixed; unsigned int events_mask; int events_mask_len; + unsigned int pebs_vmx :1; }; /*