2022-07-18 14:18:09

by Maxim Levitsky

[permalink] [raw]
Subject: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

clear_cpu_cap(&boot_cpu_data) is very similar to setup_clear_cpu_cap
except that the latter also sets a bit in 'cpu_caps_cleared' which
later clears the same cap in secondary cpus, which is likely
what is meant here.

Fixes: 47125db27e47 ("perf/x86/intel/lbr: Support Architectural LBR")

Reviewed-by: Kan Liang <[email protected]>
Signed-off-by: Maxim Levitsky <[email protected]>
---
arch/x86/events/intel/lbr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
index 13179f31fe10fa..b08715172309a7 100644
--- a/arch/x86/events/intel/lbr.c
+++ b/arch/x86/events/intel/lbr.c
@@ -1860,7 +1860,7 @@ void __init intel_pmu_arch_lbr_init(void)
return;

clear_arch_lbr:
- clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR);
+ setup_clear_cpu_cap(X86_FEATURE_ARCH_LBR);
}

/**
--
2.34.3


2022-09-19 14:32:52

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

On Mon, Jul 18, 2022 at 05:11:19PM +0300, Maxim Levitsky wrote:
> clear_cpu_cap(&boot_cpu_data) is very similar to setup_clear_cpu_cap
> except that the latter also sets a bit in 'cpu_caps_cleared' which
> later clears the same cap in secondary cpus, which is likely
> what is meant here.
>
> Fixes: 47125db27e47 ("perf/x86/intel/lbr: Support Architectural LBR")
>
> Reviewed-by: Kan Liang <[email protected]>
> Signed-off-by: Maxim Levitsky <[email protected]>
> ---
> arch/x86/events/intel/lbr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
> index 13179f31fe10fa..b08715172309a7 100644
> --- a/arch/x86/events/intel/lbr.c
> +++ b/arch/x86/events/intel/lbr.c
> @@ -1860,7 +1860,7 @@ void __init intel_pmu_arch_lbr_init(void)
> return;
>
> clear_arch_lbr:
> - clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR);
> + setup_clear_cpu_cap(X86_FEATURE_ARCH_LBR);

setup_clear_cpu_cap() has a very specific purpose - see
apply_forced_caps().

This whole call sequence is an early_initcall() which is way after the
whole CPU features picking apart happens.

So what is actually this fixing?

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2022-09-20 08:26:13

by Maxim Levitsky

[permalink] [raw]
Subject: Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

On Mon, 2022-09-19 at 16:31 +0200, Borislav Petkov wrote:
> On Mon, Jul 18, 2022 at 05:11:19PM +0300, Maxim Levitsky wrote:
> > clear_cpu_cap(&boot_cpu_data) is very similar to setup_clear_cpu_cap
> > except that the latter also sets a bit in 'cpu_caps_cleared' which
> > later clears the same cap in secondary cpus, which is likely
> > what is meant here.
> >
> > Fixes: 47125db27e47 ("perf/x86/intel/lbr: Support Architectural LBR")
> >
> > Reviewed-by: Kan Liang <[email protected]>
> > Signed-off-by: Maxim Levitsky <[email protected]>
> > ---
> >  arch/x86/events/intel/lbr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
> > index 13179f31fe10fa..b08715172309a7 100644
> > --- a/arch/x86/events/intel/lbr.c
> > +++ b/arch/x86/events/intel/lbr.c
> > @@ -1860,7 +1860,7 @@ void __init intel_pmu_arch_lbr_init(void)
> >         return;
> >  
> >  clear_arch_lbr:
> > -       clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR);
> > +       setup_clear_cpu_cap(X86_FEATURE_ARCH_LBR);
>
> setup_clear_cpu_cap() has a very specific purpose - see
> apply_forced_caps().
>
> This whole call sequence is an early_initcall() which is way after the
> whole CPU features picking apart happens.
>
> So what is actually this fixing?
>

If I understand that correctly, the difference between clear_cpu_cap and setup_clear_cpu_cap
is that setup_clear_cpu_cap should be called early when only the boot cpu is running and it

1. works on 'boot_cpu_data' which represents the boot cpu.
2. sets a bit in 'cpu_caps_cleared' which are later applied to all CPUs, including these that are hotplugged.

On the other hand the clear_cpu_cap just affects the given 'struct cpuinfo_x86'.

Call of 'clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR)' is weird since it still affects 'boot_cpu_data'
but doesn't affect 'cpu_caps_cleared'

I assumed that this was a mistake and the intention was to disable the feature on all CPUs.

I need this patch because in the next patch, I change the clear_cpu_cap such as it detects being
called on boot_cpu_data and in this case also clears bits in 'cpu_caps_cleared', thus
while this patch does introduce a functional change, the next patch doesn't since this is the only
place where clear_cpu_cap is called explicitly on 'boot_cpu_data'


I do now notice that initcalls are run after smp is initialized, which means that this code doesn't really
disable the CPUID feature on all CPUs at all.

Maybe we can drop the call instead, which does seem to be wrong?

Best regards,
Maxim Levitsky




2022-09-26 14:57:58

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

On Tue, Sep 20, 2022 at 11:20:47AM +0300, Maxim Levitsky wrote:
> If I understand that correctly, the difference between clear_cpu_cap and setup_clear_cpu_cap
> is that setup_clear_cpu_cap should be called early when only the boot cpu is running and it
>
> 1. works on 'boot_cpu_data' which represents the boot cpu.
> 2. sets a bit in 'cpu_caps_cleared' which are later applied to all CPUs, including these that are hotplugged.

Yes.

> On the other hand the clear_cpu_cap just affects the given 'struct cpuinfo_x86'.

Yes.

> Call of 'clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR)' is weird since it still affects 'boot_cpu_data'
> but doesn't affect 'cpu_caps_cleared'

Yes.

> I assumed that this was a mistake and the intention was to disable the feature on all CPUs.

peterz says yes.

> I need this patch because in the next patch, I change the clear_cpu_cap such as it detects being
> called on boot_cpu_data and in this case also clears bits in 'cpu_caps_cleared', thus
> while this patch does introduce a functional change, the next patch doesn't since this is the only
> place where clear_cpu_cap is called explicitly on 'boot_cpu_data'

This is not needed - this patch doing setup_clear_cpu_cap() should suffice.

But, there must be something you're fixing with this. Which is it? Some
weird virt config?

> I do now notice that initcalls are run after smp is initialized, which
> means that this code doesn't really disable the CPUID feature on all
> CPUs at all.

Well, not exactly. There's do_pre_smp_calls() which is where the
early_initcall() thing is run.

So setup_clear_cpu_cap() will make sure that the feature bit is cleared
when the APs come online.

Do you have a virt configuration where you can test this case where the
feature flag is clear on all CPUs when it fails?

I.e., "arch_lbr" will disappear in /proc/cpuinfo completely.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2022-09-28 10:55:51

by Maxim Levitsky

[permalink] [raw]
Subject: Re: [PATCH v2 1/5] perf/x86/intel/lbr: use setup_clear_cpu_cap instead of clear_cpu_cap

On Mon, 2022-09-26 at 15:12 +0200, Borislav Petkov wrote:
> On Tue, Sep 20, 2022 at 11:20:47AM +0300, Maxim Levitsky wrote:
> > If I understand that correctly, the difference between clear_cpu_cap and setup_clear_cpu_cap
> > is that setup_clear_cpu_cap should be called early when only the boot cpu is running and it
> >
> > 1. works on 'boot_cpu_data' which represents the boot cpu.
> > 2. sets a bit in 'cpu_caps_cleared' which are later applied to all CPUs, including these that are hotplugged.
>
> Yes.
>
> > On the other hand the clear_cpu_cap just affects the given 'struct cpuinfo_x86'.
>
> Yes.
>
> > Call of 'clear_cpu_cap(&boot_cpu_data, X86_FEATURE_ARCH_LBR)' is weird since it still affects 'boot_cpu_data'
> > but doesn't affect 'cpu_caps_cleared'
>
> Yes.
>
> > I assumed that this was a mistake and the intention was to disable the feature on all CPUs.
>
> peterz says yes.
>
> > I need this patch because in the next patch, I change the clear_cpu_cap such as it detects being
> > called on boot_cpu_data and in this case also clears bits in 'cpu_caps_cleared', thus
> > while this patch does introduce a functional change, the next patch doesn't since this is the only
> > place where clear_cpu_cap is called explicitly on 'boot_cpu_data'
>
> This is not needed - this patch doing setup_clear_cpu_cap() should suffice.
>
> But, there must be something you're fixing with this. Which is it? Some
> weird virt config?

Patches 1-3 don't fix anything - these are just refactoring to make the code simplier.

This particular patch is done to enable the refactoring in the next patch by removing an
(hopefully broken) outlier.


Patch 4 is small fix in the sense that it allows the warning from the current cpuid filtering code
to be seen (it is supressed in early code, but then it doesn't usually happen again, so no warning
is printed at all)

Patch 5 is the main fix - it makes the kernel to be tolerant to a broken CPUID config
(coming hopefully from hypervisor),
where you have a feature (AVX2 in my case) but not a feature on which this feature depends (AVX).


>
> > I do now notice that initcalls are run after smp is initialized, which
> > means that this code doesn't really disable the CPUID feature on all
> > CPUs at all.
>
> Well, not exactly. There's do_pre_smp_calls() which is where the
> early_initcall() thing is run.


Aha! I was reading the 'do_initcalls()' code and thought that it
goes over all initcalls.

note that it turns out that this function is called 'do_pre_smp_initcalls()'.

>
> So setup_clear_cpu_cap() will make sure that the feature bit is cleared
> when the APs come online.
>
> Do you have a virt configuration where you can test this case where the
> feature flag is clear on all CPUs when it fails?

This needs the arch lbrs which aren't yet supported by KVM (there are patches
on the mailing list), plus I need a hardware which supportes them, of which
I don't know even if intel released any yet.

I can hack the code/KVM though to simulate enough of it to see if this failback
happens.

Besides that, anything else I should do to to see that patch series merged?

Thanks,
Best regards,
Maxim Levitsky

>
> I.e., "arch_lbr" will disappear in /proc/cpuinfo completely.
>
> Thx.
>