Messages in this thread | | | Date | Fri, 7 Jan 2022 12:13:29 +0800 | From | Like Xu <> | Subject | Re: [DROP PATCH v2] KVM: x86/pmu: Make top-down.slots event unavailable in supported leaf |
| |
On 7/1/2022 4:12 am, Jim Mattson wrote: > On Thu, Jan 6, 2022 at 10:09 AM Sean Christopherson <seanjc@google.com> wrote: >> >> On Thu, Jan 06, 2022, Like Xu wrote: >>> From: Like Xu <likexu@tencent.com> >>> >>> When we choose to disable the fourth fixed counter TOPDOWN.SLOTS, >>> we also need to comply with the specification and set 0AH.EBX.[bit 7] >>> to 1 if the guest (e.g. on the ICX) has a value of 0AH.EAX[31:24] > 7. >>> >>> Fixes: 2e8cd7a3b8287 ("kvm: x86: limit the maximum number of vPMU fixed counters to 3") >>> Signed-off-by: Like Xu <likexu@tencent.com> >>> --- >>> v1 -> v2 Changelog: >>> - Make it simpler to keep forward compatibility; (Sean) >>> - Wrap related comment at ~80 chars; (Sean) >>> >>> Previous: >>> https://lore.kernel.org/kvm/20220105050711.67280-1-likexu@tencent.com/ >>> >>> arch/x86/kvm/cpuid.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >>> index 0b920e12bb6d..4fe17a537084 100644 >>> --- a/arch/x86/kvm/cpuid.c >>> +++ b/arch/x86/kvm/cpuid.c >>> @@ -782,6 +782,18 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) >>> eax.split.mask_length = cap.events_mask_len; >>> >>> edx.split.num_counters_fixed = min(cap.num_counters_fixed, MAX_FIXED_COUNTERS); >>> + >>> + /* >>> + * The 8th Intel architectural event (Topdown Slots) will be supported >> >> Nit, the "8th" part is unnecessary information. >> >>> + * if the 4th fixed counter exists && EAX[31:24] > 7 && EBX[7] = 0. >>> + * >>> + * Currently, KVM needs to set EAX[31:24] < 8 or EBX[7] == 1 >>> + * to make this event unavailable in a consistent way. >>> + */ >> >> This comment is now slightly stale. It also doesn't say why the event is made >> unavailable. >> >>> + if (edx.split.num_counters_fixed < 4 && >> >> Rereading the changelog and the changelog of the Fixed commit, I don't think KVM >> should bother checking num_counters_fixed. IIUC, cap.events_mask[7] should already >> be '1' if there are less than 4 fixed counters in hardware, but at the same time >> there's no harm in being a bit overzealous. That would help simplifiy the comment >> as there's no need to explain why num_counters_fixed is checked, e.g. the fact that >> Topdown Slots uses the 4th fixed counter is irrelevant with respect to the legality >> of setting EBX[7]=1 to hide an unsupported event. > > I was under the impression that the CPUID.0AH:EBX bits were > independent of the fixed counters. That is, if CPUID.0AH:EAX[31:24] > > 7 and CPUID.0AH:EBX[7] is clear, then one should be able to program a > general purpose counter with event selector A4H and umask 01H, > regardless of whether or not fixed counter 4 exists.
The impression stepped on my toes. This patch is pointless and magnifies my clumsiness.
Now the self-contradiction comes to the CLX and older platforms,
0xA. EAX. Bits 31 - 24: Length of EBX bit vector to enumerate architectural performance monitoring events. Architectural event x is supported if EBX[x]=0 && EAX[31:24]>x.
however actually for CLX, its CPUID.0xA.EBX[7] = 0 && EAX[31:24] = 0x7 and we can do exactly the following on a CLX without fixed ctr3
$ perf stat -e cpu/event=0xa4,umask=0x01/ ls Performance counter stats for 'ls': 2,448,185 cpu/event=0xa4,umask=0x01/
So in the context of Intel, we have a performance event that can share the architectural encoding, but it is not an architectural-defined event.
This is strange, did I miss something somewhere ?
> >> >> /* >> * Hide Intel's Topdown Slots architectural event, it's not yet >> * supported by KVM. >> */ >> if (eax.split.mask_length > 7) >> cap.events_mask |= BIT_ULL(7); >> >>> + eax.split.mask_length > 7) >>> + cap.events_mask |= BIT_ULL(7); >>> + >>> edx.split.bit_width_fixed = cap.bit_width_fixed; >>> if (cap.version) >>> edx.split.anythread_deprecated = 1; >>> -- >>> 2.33.1 >>>
| |