Messages in this thread | | | Date | Thu, 18 Apr 2024 14:05:49 +0200 | From | Robert Richter <> | Subject | Re: [PATCH 2/4] perf/x86/ibs: Use CPUID region helper |
| |
On 16.04.24 19:48:50, Borislav Petkov wrote: > On Tue, Apr 16, 2024 at 08:23:58AM -0700, Dave Hansen wrote: > > When I was looking at it, I (maybe wrongly) assumed that 0x8000001b and > > X86_FEATURE_IBS were independent for a reason. Because, oddly enough: > > > > #define IBS_CAPS_DEFAULT (IBS_CAPS_AVAIL \ > > | IBS_CAPS_FETCHSAM \ > > | IBS_CAPS_OPSAM) > > > > So, if the CPU enumerates X86_FEATURE_IBS but has a > > max_level<IBS_CPUID_FEATURES then it assumes there's a working IBS > > because the software-inserted IBS_CAPS_DEFAULT has IBS_CAPS_AVAIL set. > > Right, that's why I added Robert. I found this in a F10h doc (old > Greyhound CPU rust): > > "CPUID Fn8000_0001_ECX Feature Identifiers > > ... > > 10: IBS: Instruction Based Sampling = 1. > > ... > > CPUID Fn8000_001B Instruction Based Sampling Identifiers > > ... > > IBSFFV. IBS feature flags valid. Revision B = 0. Revision C = 1." > > which makes this look like some hack to fix broken CPUID IBS reporting.
There is the X86_FEATURE_IBS bit (Fn8000_0001_ECX, bit 10) which is available from the beginning of IBS (all Fam10h production releases, revB onwards).
And right, IBS_CPUID_FEATURES (CPUID Fn8000_001B) was introduced with revC. The capabilities of revB are set in IBS_CAPS_DEFAULT.
It doesn't look broken to me, simply the ibs caps field was introduced later which can be determined checking the return code of get_cpuid_region_leaf().
> > And if it is that, I don't think we care, frankly, because revB is > ooold. Mine is somewhere in the basement on some old board which got > bricked so I don't know even if I could use it anymore. > > And I'm not even planing to - that CPU is almost 20 years old and no one > cares whether it can even do IBS. > > So I wouldn't mind at all if we simplify this code for the sake of it. > I don't think anyone would care or notice. > > But let's see what Robert says first...
My preference would be:
[...] if (!get_cpuid_region_leaf(IBS_CPUID_FEATURES, CPUID_EAX, &caps)) return IBS_CAPS_DEFAULT;
if (caps & IBS_CAPS_AVAIL) return caps;
return 0; [...]
Not too complex?
This slightly modifies the functionality so that 0 is return if !IBS_CAPS_AVAIL (instead of IBS_CAPS_DEFAULT). The feature could be disabled then by overriding IBS_CAPS_AVAIL by just clearing the ibs leaf, valid even for newer systems.
Thanks,
-Robert
| |