Re: [PATCH 2/4] perf/x86/ibs: Use CPUID region helper

[Robert Richter]

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