| Message ID | 20241125132029.7241-8-patryk.wlazlyn@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | SRF: Fix offline CPU preventing pc6 entry | expand |
On Mon, Nov 25, 2024 at 2:21 PM Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com> wrote: > > Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com> The changes below look good to me, but the patch needs a changelog which should explain why it is needed or useful. In this particular case, you want to change the code ordering in native_play_dead() so that it calls cpuidle_play_dead() first and only fall back to anything else if that fails. In particular, this needs to work when intel_idle is not in use and the entry method for at least one idle state in the _CST return package for at least one CPU is ACPI_CSTATE_FFH, so this case needs to be added to acpi_idle_play_dead(). You may also make a note in the changelog that had there been a non-Intel x86 platform with a _CST returning idle states where CPU is ACPI_CSTATE_FFH had been the entry method, it wouldn't have been handled correctly today (but this is academic because of the lack of such platforms). Also, this can be the first patch in your series if [1-3/7] are dropped. > --- > arch/x86/include/asm/cpufeatures.h | 1 + > arch/x86/kernel/acpi/cstate.c | 9 +++++++++ > drivers/acpi/processor_idle.c | 2 ++ > include/acpi/processor.h | 5 +++++ > 4 files changed, 17 insertions(+) > > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h > index ea33439a5d00..1da5e08de257 100644 > --- a/arch/x86/include/asm/cpufeatures.h > +++ b/arch/x86/include/asm/cpufeatures.h > @@ -236,6 +236,7 @@ > #define X86_FEATURE_PVUNLOCK ( 8*32+20) /* PV unlock function */ > #define X86_FEATURE_VCPUPREEMPT ( 8*32+21) /* PV vcpu_is_preempted function */ > #define X86_FEATURE_TDX_GUEST ( 8*32+22) /* "tdx_guest" Intel Trust Domain Extensions Guest */ > +#define X86_FEATURE_NO_MWAIT_OFFLINE ( 8*32+23) /* Don't use MWAIT states for offlined CPUs */ > > /* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */ > #define X86_FEATURE_FSGSBASE ( 9*32+ 0) /* "fsgsbase" RDFSBASE, WRFSBASE, RDGSBASE, WRGSBASE instructions*/ > diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c > index f3ffd0a3a012..c80a3e6dba5f 100644 > --- a/arch/x86/kernel/acpi/cstate.c > +++ b/arch/x86/kernel/acpi/cstate.c > @@ -204,6 +204,15 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu, > } > EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_probe); > > +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx) > +{ > + unsigned int cpu = smp_processor_id(); > + struct cstate_entry *percpu_entry; > + > + percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu); > + mwait_play_dead_with_hint(percpu_entry->states[cx->index].eax); > +} > + > void __cpuidle acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx) > { > unsigned int cpu = smp_processor_id(); > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index 698897b29de2..586cc7d1d8aa 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -590,6 +590,8 @@ static void acpi_idle_play_dead(struct cpuidle_device *dev, int index) > raw_safe_halt(); > else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) { > io_idle(cx->address); > + } else if (cx->entry_method == ACPI_CSTATE_FFH) { > + acpi_processor_ffh_play_dead(cx); > } else > return; > } > diff --git a/include/acpi/processor.h b/include/acpi/processor.h > index a17e97e634a6..63a37e72b721 100644 > --- a/include/acpi/processor.h > +++ b/include/acpi/processor.h > @@ -280,6 +280,7 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu, > struct acpi_processor_cx *cx, > struct acpi_power_register *reg); > void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cstate); > +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx); > #else > static inline void acpi_processor_power_init_bm_check(struct > acpi_processor_flags > @@ -300,6 +301,10 @@ static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx > { > return; > } > +static inline void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx) > +{ > + return; > +} > #endif > > static inline int call_on_cpu(int cpu, long (*fn)(void *), void *arg, > -- > 2.47.0 > >
On Mon, Nov 25, 2024 at 2:41 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Mon, Nov 25, 2024 at 2:21 PM Patryk Wlazlyn > <patryk.wlazlyn@linux.intel.com> wrote: > > > > Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com> > > The changes below look good to me, but the patch needs a changelog > which should explain why it is needed or useful. > > In this particular case, you want to change the code ordering in > native_play_dead() so that it calls cpuidle_play_dead() first and only > fall back to anything else if that fails. > > In particular, this needs to work when intel_idle is not in use and > the entry method for at least one idle state in the _CST return > package for at least one CPU is ACPI_CSTATE_FFH, so this case needs to > be added to acpi_idle_play_dead(). > > You may also make a note in the changelog that had there been a > non-Intel x86 platform with a _CST returning idle states where CPU is > ACPI_CSTATE_FFH had been the entry method, it wouldn't have been > handled correctly today (but this is academic because of the lack of > such platforms). > > Also, this can be the first patch in your series if [1-3/7] are dropped. Ah sorry, not quite. You need to introduce mwait_play_dead_with_hint() first of course, so it could be the second patch in your series and the intel_idle one would be the third. > > --- > > arch/x86/include/asm/cpufeatures.h | 1 + > > arch/x86/kernel/acpi/cstate.c | 9 +++++++++ > > drivers/acpi/processor_idle.c | 2 ++ > > include/acpi/processor.h | 5 +++++ > > 4 files changed, 17 insertions(+) > > > > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h > > index ea33439a5d00..1da5e08de257 100644 > > --- a/arch/x86/include/asm/cpufeatures.h > > +++ b/arch/x86/include/asm/cpufeatures.h > > @@ -236,6 +236,7 @@ > > #define X86_FEATURE_PVUNLOCK ( 8*32+20) /* PV unlock function */ > > #define X86_FEATURE_VCPUPREEMPT ( 8*32+21) /* PV vcpu_is_preempted function */ > > #define X86_FEATURE_TDX_GUEST ( 8*32+22) /* "tdx_guest" Intel Trust Domain Extensions Guest */ > > +#define X86_FEATURE_NO_MWAIT_OFFLINE ( 8*32+23) /* Don't use MWAIT states for offlined CPUs */ > > > > /* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */ > > #define X86_FEATURE_FSGSBASE ( 9*32+ 0) /* "fsgsbase" RDFSBASE, WRFSBASE, RDGSBASE, WRGSBASE instructions*/ > > diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c > > index f3ffd0a3a012..c80a3e6dba5f 100644 > > --- a/arch/x86/kernel/acpi/cstate.c > > +++ b/arch/x86/kernel/acpi/cstate.c > > @@ -204,6 +204,15 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu, > > } > > EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_probe); > > > > +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx) > > +{ > > + unsigned int cpu = smp_processor_id(); > > + struct cstate_entry *percpu_entry; > > + > > + percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu); > > + mwait_play_dead_with_hint(percpu_entry->states[cx->index].eax); > > +} > > + > > void __cpuidle acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx) > > { > > unsigned int cpu = smp_processor_id(); > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > > index 698897b29de2..586cc7d1d8aa 100644 > > --- a/drivers/acpi/processor_idle.c > > +++ b/drivers/acpi/processor_idle.c > > @@ -590,6 +590,8 @@ static void acpi_idle_play_dead(struct cpuidle_device *dev, int index) > > raw_safe_halt(); > > else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) { > > io_idle(cx->address); > > + } else if (cx->entry_method == ACPI_CSTATE_FFH) { > > + acpi_processor_ffh_play_dead(cx); > > } else > > return; > > } > > diff --git a/include/acpi/processor.h b/include/acpi/processor.h > > index a17e97e634a6..63a37e72b721 100644 > > --- a/include/acpi/processor.h > > +++ b/include/acpi/processor.h > > @@ -280,6 +280,7 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu, > > struct acpi_processor_cx *cx, > > struct acpi_power_register *reg); > > void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cstate); > > +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx); > > #else > > static inline void acpi_processor_power_init_bm_check(struct > > acpi_processor_flags > > @@ -300,6 +301,10 @@ static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx > > { > > return; > > } > > +static inline void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx) > > +{ > > + return; > > +} > > #endif > > > > static inline int call_on_cpu(int cpu, long (*fn)(void *), void *arg, > > -- > > 2.47.0 > > > >
Hello Patryk, On Mon, Nov 25, 2024 at 02:20:27PM +0100, Patryk Wlazlyn wrote: > Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com> The patch looks good to me. But this needs a more detailed changelog. -- Thanks and Regards gautham. > --- > arch/x86/include/asm/cpufeatures.h | 1 + > arch/x86/kernel/acpi/cstate.c | 9 +++++++++ > drivers/acpi/processor_idle.c | 2 ++ > include/acpi/processor.h | 5 +++++ > 4 files changed, 17 insertions(+) > > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h > index ea33439a5d00..1da5e08de257 100644 > --- a/arch/x86/include/asm/cpufeatures.h > +++ b/arch/x86/include/asm/cpufeatures.h > @@ -236,6 +236,7 @@ > #define X86_FEATURE_PVUNLOCK ( 8*32+20) /* PV unlock function */ > #define X86_FEATURE_VCPUPREEMPT ( 8*32+21) /* PV vcpu_is_preempted function */ > #define X86_FEATURE_TDX_GUEST ( 8*32+22) /* "tdx_guest" Intel Trust Domain Extensions Guest */ > +#define X86_FEATURE_NO_MWAIT_OFFLINE ( 8*32+23) /* Don't use MWAIT states for offlined CPUs */ > > /* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */ > #define X86_FEATURE_FSGSBASE ( 9*32+ 0) /* "fsgsbase" RDFSBASE, WRFSBASE, RDGSBASE, WRGSBASE instructions*/ > diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c > index f3ffd0a3a012..c80a3e6dba5f 100644 > --- a/arch/x86/kernel/acpi/cstate.c > +++ b/arch/x86/kernel/acpi/cstate.c > @@ -204,6 +204,15 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu, > } > EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_probe); > > +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx) > +{ > + unsigned int cpu = smp_processor_id(); > + struct cstate_entry *percpu_entry; > + > + percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu); > + mwait_play_dead_with_hint(percpu_entry->states[cx->index].eax); > +} > + > void __cpuidle acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx) > { > unsigned int cpu = smp_processor_id(); > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index 698897b29de2..586cc7d1d8aa 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -590,6 +590,8 @@ static void acpi_idle_play_dead(struct cpuidle_device *dev, int index) > raw_safe_halt(); > else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) { > io_idle(cx->address); > + } else if (cx->entry_method == ACPI_CSTATE_FFH) { > + acpi_processor_ffh_play_dead(cx); > } else > return; > } > diff --git a/include/acpi/processor.h b/include/acpi/processor.h > index a17e97e634a6..63a37e72b721 100644 > --- a/include/acpi/processor.h > +++ b/include/acpi/processor.h > @@ -280,6 +280,7 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu, > struct acpi_processor_cx *cx, > struct acpi_power_register *reg); > void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cstate); > +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx); > #else > static inline void acpi_processor_power_init_bm_check(struct > acpi_processor_flags > @@ -300,6 +301,10 @@ static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx > { > return; > } > +static inline void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx) > +{ > + return; > +} > #endif > > static inline int call_on_cpu(int cpu, long (*fn)(void *), void *arg, > -- > 2.47.0 >
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h index ea33439a5d00..1da5e08de257 100644 --- a/arch/x86/include/asm/cpufeatures.h +++ b/arch/x86/include/asm/cpufeatures.h @@ -236,6 +236,7 @@ #define X86_FEATURE_PVUNLOCK ( 8*32+20) /* PV unlock function */ #define X86_FEATURE_VCPUPREEMPT ( 8*32+21) /* PV vcpu_is_preempted function */ #define X86_FEATURE_TDX_GUEST ( 8*32+22) /* "tdx_guest" Intel Trust Domain Extensions Guest */ +#define X86_FEATURE_NO_MWAIT_OFFLINE ( 8*32+23) /* Don't use MWAIT states for offlined CPUs */ /* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */ #define X86_FEATURE_FSGSBASE ( 9*32+ 0) /* "fsgsbase" RDFSBASE, WRFSBASE, RDGSBASE, WRGSBASE instructions*/ diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c index f3ffd0a3a012..c80a3e6dba5f 100644 --- a/arch/x86/kernel/acpi/cstate.c +++ b/arch/x86/kernel/acpi/cstate.c @@ -204,6 +204,15 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu, } EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_probe); +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx) +{ + unsigned int cpu = smp_processor_id(); + struct cstate_entry *percpu_entry; + + percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu); + mwait_play_dead_with_hint(percpu_entry->states[cx->index].eax); +} + void __cpuidle acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx) { unsigned int cpu = smp_processor_id(); diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 698897b29de2..586cc7d1d8aa 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -590,6 +590,8 @@ static void acpi_idle_play_dead(struct cpuidle_device *dev, int index) raw_safe_halt(); else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) { io_idle(cx->address); + } else if (cx->entry_method == ACPI_CSTATE_FFH) { + acpi_processor_ffh_play_dead(cx); } else return; } diff --git a/include/acpi/processor.h b/include/acpi/processor.h index a17e97e634a6..63a37e72b721 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -280,6 +280,7 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu, struct acpi_processor_cx *cx, struct acpi_power_register *reg); void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cstate); +void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx); #else static inline void acpi_processor_power_init_bm_check(struct acpi_processor_flags @@ -300,6 +301,10 @@ static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx { return; } +static inline void acpi_processor_ffh_play_dead(struct acpi_processor_cx *cx) +{ + return; +} #endif static inline int call_on_cpu(int cpu, long (*fn)(void *), void *arg,
Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com> --- arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/kernel/acpi/cstate.c | 9 +++++++++ drivers/acpi/processor_idle.c | 2 ++ include/acpi/processor.h | 5 +++++ 4 files changed, 17 insertions(+)