* [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend
@ 2015-08-17 8:29 Chen Yu
2015-08-17 10:11 ` Ingo Molnar
0 siblings, 1 reply; 9+ messages in thread
From: Chen Yu @ 2015-08-17 8:29 UTC (permalink / raw)
To: rjw, pavel, tglx, mingo, hpa
Cc: rui.zhang, x86, linux-pm, linux-kernel, Chen Yu
A bug is reported(https://bugzilla.redhat.com/show_bug.cgi?id=1227208)
that, after resuming from S3, CPU is working at a low speed.
After investigation, it is found that, BIOS has modified the value
of THERM_CONTROL register during S3, changes it from 0 to 0x10,
while the latter means CPU can only get 25% of the Duty Cycle,
and this caused the problem.
Simple scenario to reproduce:
1.Boot up system
2.Get MSR with address 0x19a, it should output 0
3.Put system into sleep, then wake up
4.Get MSR with address 0x19a, it should output 0(actual it outputs 0x10)
Although this is a BIOS issue, it would be more robust for linux to deal
with this situation. This patch fixes this issue by saving/restoring
THERM_CONTROL(now called CLOCK_MODULATION) register on suspend/resume.
Tested-by: Marcin Kaszewski <marcin.kaszewski@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
arch/x86/include/asm/suspend_64.h | 1 +
arch/x86/power/cpu.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/arch/x86/include/asm/suspend_64.h b/arch/x86/include/asm/suspend_64.h
index 7ebf0eb..b9f5591 100644
--- a/arch/x86/include/asm/suspend_64.h
+++ b/arch/x86/include/asm/suspend_64.h
@@ -25,6 +25,7 @@ struct saved_context {
u64 misc_enable;
bool misc_enable_saved;
unsigned long efer;
+ unsigned long clock_modulation;
u16 gdt_pad; /* Unused */
struct desc_ptr gdt_desc;
u16 idt_pad;
diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 9ab5279..f82577b 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -97,6 +97,7 @@ static void __save_processor_state(struct saved_context *ctxt)
mtrr_save_fixed_ranges(NULL);
rdmsrl(MSR_EFER, ctxt->efer);
+ rdmsrl(MSR_IA32_THERM_CONTROL, ctxt->clock_modulation);
#endif
/*
@@ -178,6 +179,7 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
#else
/* CONFIG X86_64 */
wrmsrl(MSR_EFER, ctxt->efer);
+ wrmsrl(MSR_IA32_THERM_CONTROL, ctxt->clock_modulation);
write_cr8(ctxt->cr8);
__write_cr4(ctxt->cr4);
#endif
--
1.8.4.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend
2015-08-17 8:29 [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend Chen Yu
@ 2015-08-17 10:11 ` Ingo Molnar
2015-08-17 11:43 ` Chen, Yu C
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ingo Molnar @ 2015-08-17 10:11 UTC (permalink / raw)
To: Chen Yu
Cc: rjw, pavel, tglx, mingo, hpa, rui.zhang, x86, linux-pm, linux-kernel
* Chen Yu <yu.c.chen@intel.com> wrote:
> A bug is reported(https://bugzilla.redhat.com/show_bug.cgi?id=1227208)
> that, after resuming from S3, CPU is working at a low speed.
> After investigation, it is found that, BIOS has modified the value
> of THERM_CONTROL register during S3, changes it from 0 to 0x10,
> while the latter means CPU can only get 25% of the Duty Cycle,
> and this caused the problem.
>
> Simple scenario to reproduce:
> 1.Boot up system
> 2.Get MSR with address 0x19a, it should output 0
> 3.Put system into sleep, then wake up
> 4.Get MSR with address 0x19a, it should output 0(actual it outputs 0x10)
>
> Although this is a BIOS issue, it would be more robust for linux to deal
> with this situation. This patch fixes this issue by saving/restoring
> THERM_CONTROL(now called CLOCK_MODULATION) register on suspend/resume.
>
> Tested-by: Marcin Kaszewski <marcin.kaszewski@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
> arch/x86/include/asm/suspend_64.h | 1 +
> arch/x86/power/cpu.c | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/arch/x86/include/asm/suspend_64.h b/arch/x86/include/asm/suspend_64.h
> index 7ebf0eb..b9f5591 100644
> --- a/arch/x86/include/asm/suspend_64.h
> +++ b/arch/x86/include/asm/suspend_64.h
> @@ -25,6 +25,7 @@ struct saved_context {
> u64 misc_enable;
> bool misc_enable_saved;
> unsigned long efer;
> + unsigned long clock_modulation;
> u16 gdt_pad; /* Unused */
> struct desc_ptr gdt_desc;
> u16 idt_pad;
> diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
> index 9ab5279..f82577b 100644
> --- a/arch/x86/power/cpu.c
> +++ b/arch/x86/power/cpu.c
> @@ -97,6 +97,7 @@ static void __save_processor_state(struct saved_context *ctxt)
> mtrr_save_fixed_ranges(NULL);
>
> rdmsrl(MSR_EFER, ctxt->efer);
> + rdmsrl(MSR_IA32_THERM_CONTROL, ctxt->clock_modulation);
So what your changelog fails to mention:
- You only add this code to the 64-bit kernel. Are 32-bit kernels not affected?
- the MSR read is done unconditionally. Is MSR_IA32_THERM_CONTROL available
architecturally and readable (and has sensible values) on all 64-bit capable
x86 CPUs that run this code path?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend
2015-08-17 10:11 ` Ingo Molnar
@ 2015-08-17 11:43 ` Chen, Yu C
2015-08-17 13:27 ` Pavel Machek
2015-08-18 2:02 ` Yu Chen
2 siblings, 0 replies; 9+ messages in thread
From: Chen, Yu C @ 2015-08-17 11:43 UTC (permalink / raw)
To: mingo
Cc: linux-kernel, Zhang, Rui, tglx, linux-pm, pavel, x86, hpa, rjw, mingo
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1144 bytes --]
Hi, Ingo, thanks for your review,
On Mon, 2015-08-17 at 12:11 +0200, Ingo Molnar wrote:
> So what your changelog fails to mention:
>
> - You only add this code to the 64-bit kernel. Are 32-bit kernels not affected?
Yes, 32-bit kernel should also do the save/restore operation.
I'll adjust them to 64/32-bit common path.
>
> - the MSR read is done unconditionally. Is MSR_IA32_THERM_CONTROL available
> architecturally and readable (and has sensible values) on all 64-bit capable
> x86 CPUs that run this code path?
MSR_IA32_THERM_CONTROL is available on Intel Pentium 4, Xeon, Pentium M and later
processors, so I think not all the 64/32-bit capable x86 CPUs have this
register. Maybe codes like the following would be more reasonable?
save:
ctxt->clock_modulation_saved = !rdmsrl_safe(MSR_IA32_THERM_CONTROL,
&ctxt->clock_modulation);
restore:
if (ctxt->clock_modulation_saved)
wrmsrl(MSR_IA32_THERM_CONTROL, ctxt->clock_modulation);
Thanks a lot.
Best Regards,
Yu
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend
@ 2015-08-17 11:43 ` Chen, Yu C
0 siblings, 0 replies; 9+ messages in thread
From: Chen, Yu C @ 2015-08-17 11:43 UTC (permalink / raw)
To: mingo
Cc: linux-kernel, Zhang, Rui, tglx, linux-pm, pavel, x86, hpa, rjw, mingo
Hi, Ingo, thanks for your review,
On Mon, 2015-08-17 at 12:11 +0200, Ingo Molnar wrote:
> So what your changelog fails to mention:
>
> - You only add this code to the 64-bit kernel. Are 32-bit kernels not affected?
Yes, 32-bit kernel should also do the save/restore operation.
I'll adjust them to 64/32-bit common path.
>
> - the MSR read is done unconditionally. Is MSR_IA32_THERM_CONTROL available
> architecturally and readable (and has sensible values) on all 64-bit capable
> x86 CPUs that run this code path?
MSR_IA32_THERM_CONTROL is available on Intel Pentium 4, Xeon, Pentium M and later
processors, so I think not all the 64/32-bit capable x86 CPUs have this
register. Maybe codes like the following would be more reasonable?
save:
ctxt->clock_modulation_saved = !rdmsrl_safe(MSR_IA32_THERM_CONTROL,
&ctxt->clock_modulation);
restore:
if (ctxt->clock_modulation_saved)
wrmsrl(MSR_IA32_THERM_CONTROL, ctxt->clock_modulation);
Thanks a lot.
Best Regards,
Yu
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend
2015-08-17 10:11 ` Ingo Molnar
2015-08-17 11:43 ` Chen, Yu C
@ 2015-08-17 13:27 ` Pavel Machek
2015-08-18 2:23 ` Yu Chen
2015-08-18 2:02 ` Yu Chen
2 siblings, 1 reply; 9+ messages in thread
From: Pavel Machek @ 2015-08-17 13:27 UTC (permalink / raw)
To: Ingo Molnar
Cc: Chen Yu, rjw, tglx, mingo, hpa, rui.zhang, x86, linux-pm, linux-kernel
On Mon 2015-08-17 12:11:15, Ingo Molnar wrote:
>
> * Chen Yu <yu.c.chen@intel.com> wrote:
>
> > A bug is reported(https://bugzilla.redhat.com/show_bug.cgi?id=1227208)
Access denied :-(
> > that, after resuming from S3, CPU is working at a low speed.
> > After investigation, it is found that, BIOS has modified the value
> > of THERM_CONTROL register during S3, changes it from 0 to 0x10,
> > while the latter means CPU can only get 25% of the Duty Cycle,
> > and this caused the problem.
What HW is this on?
> > --- a/arch/x86/power/cpu.c
> > +++ b/arch/x86/power/cpu.c
> > @@ -97,6 +97,7 @@ static void __save_processor_state(struct saved_context *ctxt)
> > mtrr_save_fixed_ranges(NULL);
> >
> > rdmsrl(MSR_EFER, ctxt->efer);
> > + rdmsrl(MSR_IA32_THERM_CONTROL, ctxt->clock_modulation);
>
> So what your changelog fails to mention:
>
> - You only add this code to the 64-bit kernel. Are 32-bit kernels not affected?
>
> - the MSR read is done unconditionally. Is MSR_IA32_THERM_CONTROL available
> architecturally and readable (and has sensible values) on all 64-bit capable
> x86 CPUs that run this code path?
- So BIOS expects to control MSR_IA32_THERM_CONTROL . Now you suspend
in hot enironment but resume in cool one. BIOS sets up
MSR_IA32_THERM_CONTROL the right way, but you override it.
As BIOS expects to control MSR_IA32_THERM_CONTROL and machine is
kept cool, BIOS will not write new value to it, and machine will
keep running slowly.
Doing this unconditionally is asking for trouble. Blacklist entry with
affected BIOS info might be acceptable, but...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend
2015-08-17 10:11 ` Ingo Molnar
2015-08-17 11:43 ` Chen, Yu C
2015-08-17 13:27 ` Pavel Machek
@ 2015-08-18 2:02 ` Yu Chen
2 siblings, 0 replies; 9+ messages in thread
From: Yu Chen @ 2015-08-18 2:02 UTC (permalink / raw)
To: Ingo Molnar
Cc: rjw, pavel, tglx, mingo, hpa, rui.zhang, x86, linux-pm, linux-kernel
(not sure if previous reply has been sent out)
Hi, Ingo, thanks for your review,
On 08/17/2015 06:11 PM, Ingo Molnar wrote:
>
> * Chen Yu <yu.c.chen@intel.com> wrote:
>
>
> So what your changelog fails to mention:
>
> - You only add this code to the 64-bit kernel. Are 32-bit kernels not affected?
>
I missed the 32-bit case, I'll adjust them to 32/64-bit common path.
> - the MSR read is done unconditionally. Is MSR_IA32_THERM_CONTROL available
> architecturally and readable (and has sensible values) on all 64-bit capable
> x86 CPUs that run this code path?
>
MSR_IA32_THERM_CONTROL is avaliable on Intel Pentium 4, Xeon, Pentium M
and later processors, so I think not all of the 32/64-bit x86 CPUs have
this regiser. Maybe codes like this would be more reasonable?
save:
ctx->clock_modulation_saved = !rdmsrl_safe(MSR_IA32_THERM_CONTROL,
&ctxt->clock_modulation);
retore:
if (ctxt->clock_modulation_saved)
wrmsrl(MSR_IA32_THERM_CONTROL, ctxt->clock_modulation);
Best Regards,
Yu
> Thanks,
>
> Ingo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend
2015-08-17 13:27 ` Pavel Machek
@ 2015-08-18 2:23 ` Yu Chen
2015-08-18 8:02 ` Pavel Machek
0 siblings, 1 reply; 9+ messages in thread
From: Yu Chen @ 2015-08-18 2:23 UTC (permalink / raw)
To: Pavel Machek, Ingo Molnar
Cc: rjw, tglx, mingo, hpa, rui.zhang, x86, linux-pm, linux-kernel
Hi Pavel, thanks for your review,
On 08/17/2015 09:27 PM, Pavel Machek wrote:
> On Mon 2015-08-17 12:11:15, Ingo Molnar wrote:
>>
>> * Chen Yu <yu.c.chen@intel.com> wrote:
>>
>>> A bug is reported(https://bugzilla.redhat.com/show_bug.cgi?id=1227208)
>
> Access denied :-(
>
Might need to register for accessing.
>
> What HW is this on?
>
Intel Braswell and Broadwell, detail for Broadwell:
Platform: MayanCity
Processor: 2x BROADWELL BDX_EP A0 QHPR
Chipset: Wellsburg B1 QR7E
>
> - So BIOS expects to control MSR_IA32_THERM_CONTROL . Now you suspend
> in hot enironment but resume in cool one. BIOS sets up
> MSR_IA32_THERM_CONTROL the right way, but you override it.
>
> As BIOS expects to control MSR_IA32_THERM_CONTROL and machine is
> kept cool, BIOS will not write new value to it, and machine will
> keep running slowly.
>
Sorry, I can not quite catch up, do you mean we should let
BIOS modifying MSR_IA32_THERM_CONTROL and leverage linux
to adjust this value at runtime(after S3)?
> Doing this unconditionally is asking for trouble. Blacklist entry with
> affected BIOS info might be acceptable, but...
> Pavel
>
you mean a quirk here(accroding to dmi info ,etc)?
Best Regards,
Yu
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend
2015-08-18 2:23 ` Yu Chen
@ 2015-08-18 8:02 ` Pavel Machek
2015-08-18 8:54 ` Yu Chen
0 siblings, 1 reply; 9+ messages in thread
From: Pavel Machek @ 2015-08-18 8:02 UTC (permalink / raw)
To: Yu Chen
Cc: Ingo Molnar, rjw, tglx, mingo, hpa, rui.zhang, x86, linux-pm,
linux-kernel
Hi!
> >>>A bug is reported(https://bugzilla.redhat.com/show_bug.cgi?id=1227208)
> >
> >Access denied :-(
> >
> Might need to register for accessing.
> >
> >What HW is this on?
> >
> Intel Braswell and Broadwell, detail for Broadwell:
> Platform: MayanCity
> Processor: 2x BROADWELL BDX_EP A0 QHPR
> Chipset: Wellsburg B1 QR7E
So it is desktop board?
> >Doing this unconditionally is asking for trouble. Blacklist entry with
> >affected BIOS info might be acceptable, but...
> >
> you mean a quirk here(accroding to dmi info ,etc)?
Yes, please.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend
2015-08-18 8:02 ` Pavel Machek
@ 2015-08-18 8:54 ` Yu Chen
0 siblings, 0 replies; 9+ messages in thread
From: Yu Chen @ 2015-08-18 8:54 UTC (permalink / raw)
To: Pavel Machek
Cc: Ingo Molnar, rjw, tglx, mingo, hpa, rui.zhang, x86, linux-pm,
linux-kernel
Hi!
On 08/18/2015 04:02 PM, Pavel Machek wrote:
> Hi!
>
>>>>> A bug is reported(https://bugzilla.redhat.com/show_bug.cgi?id=1227208)
>>>
>>> Access denied :-(
>>>
>> Might need to register for accessing.
>>>
>>> What HW is this on?
>>>
>> Intel Braswell and Broadwell, detail for Broadwell:
>> Platform: MayanCity
>> Processor: 2x BROADWELL BDX_EP A0 QHPR
>> Chipset: Wellsburg B1 QR7E
>
> So it is desktop board?
>
It is a server of Xeon family.
>>> Doing this unconditionally is asking for trouble. Blacklist entry with
>>> affected BIOS info might be acceptable, but...
>>>
>> you mean a quirk here(accroding to dmi info ,etc)?
>
> Yes, please.
Will do. thanks
> Pavel
>
Best Regards,
Yu
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-08-18 8:50 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-17 8:29 [PATCH] x86, suspend: Save/restore THERM_CONTROL register for suspend Chen Yu
2015-08-17 10:11 ` Ingo Molnar
2015-08-17 11:43 ` Chen, Yu C
2015-08-17 11:43 ` Chen, Yu C
2015-08-17 13:27 ` Pavel Machek
2015-08-18 2:23 ` Yu Chen
2015-08-18 8:02 ` Pavel Machek
2015-08-18 8:54 ` Yu Chen
2015-08-18 2:02 ` Yu Chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.