From: Dave Hansen <[email protected]>
IBS details are enumerated in an extended CPUID leaf. But
the support has an open-coded CPUID region check. Use the
new helper to trim down the code.
Signed-off-by: Dave Hansen <[email protected]>
--
Note: this cleanup could take another form:
if (boot_cpu_data->extended_cpuid_level >= IBS_CPUID_FEATURES)
caps = cpuid_eax(IBS_CPUID_FEATURES);
that would be one fewer CPUID invocations, but one more
line of code.
---
b/arch/x86/events/amd/ibs.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff -puN arch/x86/events/amd/ibs.c~ibs-region-helpers arch/x86/events/amd/ibs.c
--- a/arch/x86/events/amd/ibs.c~ibs-region-helpers 2024-04-02 15:22:59.262912595 -0700
+++ b/arch/x86/events/amd/ibs.c 2024-04-02 15:22:59.262912595 -0700
@@ -1278,18 +1278,13 @@ static __init int perf_event_ibs_init(vo
static __init u32 __get_ibs_caps(void)
{
- u32 caps;
- unsigned int max_level;
+ u32 caps = 0;
if (!boot_cpu_has(X86_FEATURE_IBS))
return 0;
- /* check IBS cpuid feature flags */
- max_level = cpuid_eax(0x80000000);
- if (max_level < IBS_CPUID_FEATURES)
- return IBS_CAPS_DEFAULT;
+ get_cpuid_region_leaf(IBS_CPUID_FEATURES, CPUID_EAX, &caps);
- caps = cpuid_eax(IBS_CPUID_FEATURES);
if (!(caps & IBS_CAPS_AVAIL))
/* cpuid flags not valid */
return IBS_CAPS_DEFAULT;
_
On Wed, Apr 03, 2024 at 08:35:11AM -0700, Dave Hansen wrote:
>
> From: Dave Hansen <[email protected]>
>
> IBS details are enumerated in an extended CPUID leaf. But
> the support has an open-coded CPUID region check. Use the
> new helper to trim down the code.
>
> Signed-off-by: Dave Hansen <[email protected]>
> --
>
> Note: this cleanup could take another form:
>
> if (boot_cpu_data->extended_cpuid_level >= IBS_CPUID_FEATURES)
> caps = cpuid_eax(IBS_CPUID_FEATURES);
>
> that would be one fewer CPUID invocations, but one more
> line of code.
> ---
>
> b/arch/x86/events/amd/ibs.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff -puN arch/x86/events/amd/ibs.c~ibs-region-helpers arch/x86/events/amd/ibs.c
> --- a/arch/x86/events/amd/ibs.c~ibs-region-helpers 2024-04-02 15:22:59.262912595 -0700
> +++ b/arch/x86/events/amd/ibs.c 2024-04-02 15:22:59.262912595 -0700
> @@ -1278,18 +1278,13 @@ static __init int perf_event_ibs_init(vo
>
> static __init u32 __get_ibs_caps(void)
> {
> - u32 caps;
> - unsigned int max_level;
> + u32 caps = 0;
>
> if (!boot_cpu_has(X86_FEATURE_IBS))
> return 0;
>
> - /* check IBS cpuid feature flags */
> - max_level = cpuid_eax(0x80000000);
> - if (max_level < IBS_CPUID_FEATURES)
> - return IBS_CAPS_DEFAULT;
> + get_cpuid_region_leaf(IBS_CPUID_FEATURES, CPUID_EAX, &caps);
I wanna say all this checking of max level is worthless because if you
have X86_FEATURE_IBS, then it is a given that you also have that
0x8000001b CPUID leaf.
Right, Bob?
Unless there was some weird thing back then with the CPUID leafs...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/16/24 08:12, Borislav Petkov wrote:
>> if (!boot_cpu_has(X86_FEATURE_IBS))
>> return 0;
>>
>> - /* check IBS cpuid feature flags */
>> - max_level = cpuid_eax(0x80000000);
>> - if (max_level < IBS_CPUID_FEATURES)
>> - return IBS_CAPS_DEFAULT;
>> + get_cpuid_region_leaf(IBS_CPUID_FEATURES, CPUID_EAX, &caps);
> I wanna say all this checking of max level is worthless because if you
> have X86_FEATURE_IBS, then it is a given that you also have that
> 0x8000001b CPUID leaf.
>
> Right, Bob?
>
> Unless there was some weird thing back then with the CPUID leafs...
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.
Actually, that even means that the new code should probably be:
u32 caps = IBS_CAPS_DEFAULT;
if (!boot_cpu_has(X86_FEATURE_IBS))
return 0;
get_cpuid_region_leaf(IBS_CPUID_FEATURES, CPUID_EAX, &caps);
to match the old.
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.
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...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
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
On Thu, Apr 18, 2024 at 02:05:49PM +0200, Robert Richter wrote:
> 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().
Right.
> My preference would be:
>
> [...]
> if (!get_cpuid_region_leaf(IBS_CPUID_FEATURES, CPUID_EAX, &caps))
Right, checking get_cpuid_region_leaf() retval should happen.
> return IBS_CAPS_DEFAULT;
>
> if (caps & IBS_CAPS_AVAIL)
> return caps;
>
> return 0;
> [...]
>
> Not too complex?
I'm wondering should we even support those old things? All revBs should
be probably dead by now. But ok, it's not like it is a lot of code or
so...
> This slightly modifies the functionality so that 0 is return if
> !IBS_CAPS_AVAIL (instead of IBS_CAPS_DEFAULT).
If !IBS_CAPS_AVAIL, then this is revB. But then you want to return
IBS_CAPS_DEFAULT there.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 22.04.24 19:20:55, Borislav Petkov wrote:
> On Thu, Apr 18, 2024 at 02:05:49PM +0200, Robert Richter wrote:
> > 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().
>
> Right.
>
> > My preference would be:
> >
> > [...]
> > if (!get_cpuid_region_leaf(IBS_CPUID_FEATURES, CPUID_EAX, &caps))
>
> Right, checking get_cpuid_region_leaf() retval should happen.
>
> > return IBS_CAPS_DEFAULT;
> >
> > if (caps & IBS_CAPS_AVAIL)
> > return caps;
> >
> > return 0;
> > [...]
> >
> > Not too complex?
>
> I'm wondering should we even support those old things? All revBs should
> be probably dead by now. But ok, it's not like it is a lot of code or
> so...
Since we check get_cpuid_region_leaf()'s return code here we know if
the cpud leaf exists and return IBS_CAPS_DEFAULT if not. That would
not change the refB behaviour.
Though I think that case is rare or even not existing, I would just
keep the implementation like that and as it was for for years.
>
> > This slightly modifies the functionality so that 0 is return if
> > !IBS_CAPS_AVAIL (instead of IBS_CAPS_DEFAULT).
>
> If !IBS_CAPS_AVAIL, then this is revB. But then you want to return
> IBS_CAPS_DEFAULT there.
No, on a rebB get_cpuid_region_leaf() would be false, meaning the
cpuid leaf is missing, function returns with IBS_CAPS_DEFAULT then.
-Robert
On Mon, Apr 22, 2024 at 10:09:57PM +0200, Robert Richter wrote:
> Since we check get_cpuid_region_leaf()'s return code here we know if
> the cpud leaf exists and return IBS_CAPS_DEFAULT if not. That would
> not change the refB behaviour.
Yes.
> Though I think that case is rare or even not existing, I would just
> keep the implementation like that and as it was for for years.
Yes.
> > > This slightly modifies the functionality so that 0 is return if
> > > !IBS_CAPS_AVAIL (instead of IBS_CAPS_DEFAULT).
> >
> > If !IBS_CAPS_AVAIL, then this is revB. But then you want to return
> > IBS_CAPS_DEFAULT there.
>
> No, on a rebB get_cpuid_region_leaf() would be false, meaning the
> cpuid leaf is missing, function returns with IBS_CAPS_DEFAULT then.
So what functionality modification do you mean then?
When will IBS_CAPS_AVAIL be not set?
GH BKDG says about that bit:
"IBSFFV. IBS feature flags valid. Revision B = 0. Revision C = 1."
so that has been set ever since on >= revC.
And on revB we'll return IBS_CAPS_DEFAULT which has IBS_CAPS_AVAIL.
IOW, I don't see how we'll return 0 if !IBS_CAPS_AVAIL because latter
doesn't happen practically with that flow.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 22.04.24 22:41:46, Borislav Petkov wrote:
> On Mon, Apr 22, 2024 at 10:09:57PM +0200, Robert Richter wrote:
> > Since we check get_cpuid_region_leaf()'s return code here we know if
> > the cpud leaf exists and return IBS_CAPS_DEFAULT if not. That would
> > not change the refB behaviour.
>
> Yes.
>
> > Though I think that case is rare or even not existing, I would just
> > keep the implementation like that and as it was for for years.
>
> Yes.
>
> > > > This slightly modifies the functionality so that 0 is return if
> > > > !IBS_CAPS_AVAIL (instead of IBS_CAPS_DEFAULT).
> > >
> > > If !IBS_CAPS_AVAIL, then this is revB. But then you want to return
> > > IBS_CAPS_DEFAULT there.
> >
> > No, on a rebB get_cpuid_region_leaf() would be false, meaning the
> > cpuid leaf is missing, function returns with IBS_CAPS_DEFAULT then.
>
> So what functionality modification do you mean then?
>
> When will IBS_CAPS_AVAIL be not set?
I mean the case where the cpuid leaf exists but IBS_CAPS_AVAIL is
clear. That could be possible with some cpuid override e.g. in virt
envs.
>
> GH BKDG says about that bit:
>
> "IBSFFV. IBS feature flags valid. Revision B = 0. Revision C = 1."
>
> so that has been set ever since on >= revC.
>
> And on revB we'll return IBS_CAPS_DEFAULT which has IBS_CAPS_AVAIL.
Yes, all this is correct.
>
> IOW, I don't see how we'll return 0 if !IBS_CAPS_AVAIL because latter
> doesn't happen practically with that flow.
Not on real hardware and if future systems not decide to enable IBS
feature bit and clear IBS_CAPS_AVAIL, which could be a valid case IMO.
Thanks,
-Robert
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
On 22.04.24 23:45:03, Borislav Petkov wrote:
> On Mon, Apr 22, 2024 at 11:30:24PM +0200, Robert Richter wrote:
> > I mean the case where the cpuid leaf exists but IBS_CAPS_AVAIL is
> > clear. That could be possible with some cpuid override e.g. in virt
> > envs.
>
> Until there is a valid use case, I don't care.
>
> > Not on real hardware and if future systems not decide to enable IBS
> > feature bit and clear IBS_CAPS_AVAIL, which could be a valid case IMO.
>
> Then they get what they ordered and get to keep the pieces.
>
> We don't support every insane configuration virt comes up with.
I think we can assume/agree that max cpuid leaf will not decrease.
That is, future implementations will contain the IBS leaf (offset
1Bh). Second, if IBS feature bit is cleared, IBS is switched
off. Third, use IBS_CAPS_DEFAULT if the IBS leaf is missing.
Now, we just need to decide one of those for the case where the IBS
cpuid leaf exists and IBS_CAPS_AVAIL is cleared:
1) Apply 0 to caps and entirely disable IBS (my proposal).
2) Apply IBS_CAPS_DEFAULT (originally intended for GH revB) and
enable IBS with the limited feature set for GH revB (current
kernel implementation).
I prefer 1) as this applies IBS_CAPS_DEFAULT only if the leaf is
missing which was the original intention of IBS_CAPS_DEFAULT but can
live with 2) as it is implemented now.
Thanks,
-Robert
On Mon, Apr 22, 2024 at 11:30:24PM +0200, Robert Richter wrote:
> I mean the case where the cpuid leaf exists but IBS_CAPS_AVAIL is
> clear. That could be possible with some cpuid override e.g. in virt
> envs.
Until there is a valid use case, I don't care.
> Not on real hardware and if future systems not decide to enable IBS
> feature bit and clear IBS_CAPS_AVAIL, which could be a valid case IMO.
Then they get what they ordered and get to keep the pieces.
We don't support every insane configuration virt comes up with.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Apr 23, 2024 at 09:45:10AM +0200, Robert Richter wrote:
> I prefer 1) as this applies IBS_CAPS_DEFAULT only if the leaf is
> missing which was the original intention of IBS_CAPS_DEFAULT but can
> live with 2) as it is implemented now.
As implemented now and be done with it. We already wasted too much time
with some hypothetical and there's bigger fish to fry so let's leave it
like it is and move on.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette