lkml.org 
[lkml]   [2024]   [Apr]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2024-04-18 14:06    [W:0.113 / U:0.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site