On Thu, Jan 06, 2022, Like Xu wrote: > From: Like Xu <likexu@xxxxxxxxxxx> > > 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@xxxxxxxxxxx> > --- > 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@xxxxxxxxxxx/ > > 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. /* * 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 >