Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1244053 > unrolled thread

[PATCH][v5] x86, suspend: Save/restore extra MSR registers for suspend

Started byChen Yu <yu.c.chen@intel.com>
First post2015-10-11 05:20 +0200
Last post2015-10-12 20:50 +0200
Articles 5 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH][v5] x86, suspend: Save/restore extra MSR registers for suspend Chen Yu <yu.c.chen@intel.com> - 2015-10-11 05:20 +0200
    Re: [PATCH][v5] x86, suspend: Save/restore extra MSR registers for  suspend Borislav Petkov <bp@suse.de> - 2015-10-12 18:40 +0200
      Re: [PATCH][v5] x86, suspend: Save/restore extra MSR registers for suspend "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-10-12 20:50 +0200
      RE: [PATCH][v5] x86, suspend: Save/restore extra MSR registers for  suspend "Chen, Yu C" <yu.c.chen@intel.com> - 2015-10-14 19:50 +0200
    Re: [PATCH][v5] x86, suspend: Save/restore extra MSR registers for suspend "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-10-12 20:50 +0200

#1244053 — [PATCH][v5] x86, suspend: Save/restore extra MSR registers for suspend

FromChen Yu <yu.c.chen@intel.com>
Date2015-10-11 05:20 +0200
Subject[PATCH][v5] x86, suspend: Save/restore extra MSR registers for suspend
Message-ID<qifkZ-4ct-1@gated-at.bofh.it>
A bug is reported(https://bugzilla.redhat.com/show_bug.cgi?id=1227208)
that, after resumed from S3, CPU is running at a low speed.
After investigation, it is found that, BIOS has modified the value
of THERM_CONTROL register during S3, and changes it from 0 to 0x10
(thus changes the clock modulation from reserved to enabled),
since value of 0x10 means CPU can only get 25% of the Duty Cycle,
this triggers the problem.

Here is a simple scenario to reproduce the issue(only for above case):
1.Boot up the system
2.Get MSR with address 0x19a, it should be 0
3.Put the system into sleep, then wake it up
4.Get MSR with address 0x19a, it should be 0(actually it shows 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 introducing a framework
to save/restore specified MSR registers(THERM_CONTROL in this case)
for suspend/resume.

When user encounters a problematic platform and needs to protect the
MSRs during suspending, he can simply add a quirk entry in
msr_save_dmi_table, and customizes MSR registers inside the quirk
callback, for example:

u32 msr_id_need_to_save[] = {MSR_ID0, MSR_ID1, MSR_ID2...};

and the quirk mechanism ensures that, once resumed from suspended,
the MSRs indicated by these IDs will be restored to their original values
before suspended.

Since both 64/32-bit kernels are affected, this patch covers 64/32-bit
common code path. And because the MSRs specified by the user might not
be available or readable in any situation, we use rdmsrl_safe to safely
save these MSRs.

Tested-by: Marcin Kaszewski <marcin.kaszewski@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
v5:
 - Rename some structures and variables for better understanding.
   Put the defination of struct saved_msr and struct msr_save_data to
   header: arch/x86/include/asm/msr.h.
   Re-implement the msr_save_context and msr_restore_context for better
   maintaining.
   Convert the msr_context_array to be dynamically allocated.
   Fix some typos in code comments.
v4:
 - Revert v3 to v2, and fix some typos in changelog/comments. 
   Use msr_info structure instead of msr_id + msr_value.
   Adjust some codes for better readability.
v3:
 - Simplify the patch to only focus on THERM_CONTROL register.
   This will make things 'just work'.
v2:
 - Cover both 64/32-bit common code path.
   Use rdmsrl_safe to safely read MSR.
   Introduce a quirk framework for save/restore specified MSR on different
   platforms.
---
 arch/x86/include/asm/msr.h        | 10 ++++
 arch/x86/include/asm/suspend_32.h |  1 +
 arch/x86/include/asm/suspend_64.h |  1 +
 arch/x86/power/cpu.c              | 98 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 110 insertions(+)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 77d8b28..5ae24ed 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -32,6 +32,16 @@ struct msr_regs_info {
 	int err;
 };
 
+struct msr_save_data {
+	bool msr_saved;
+	struct msr_info rv;
+};
+
+struct msr_context {
+	unsigned short num;
+	struct msr_save_data *msr_array;
+};
+
 static inline unsigned long long native_read_tscp(unsigned int *aux)
 {
 	unsigned long low, high;
diff --git a/arch/x86/include/asm/suspend_32.h b/arch/x86/include/asm/suspend_32.h
index d1793f0..5057f65 100644
--- a/arch/x86/include/asm/suspend_32.h
+++ b/arch/x86/include/asm/suspend_32.h
@@ -15,6 +15,7 @@ struct saved_context {
 	unsigned long cr0, cr2, cr3, cr4;
 	u64 misc_enable;
 	bool misc_enable_saved;
+	struct msr_context msr_to_save;
 	struct desc_ptr gdt_desc;
 	struct desc_ptr idt;
 	u16 ldt;
diff --git a/arch/x86/include/asm/suspend_64.h b/arch/x86/include/asm/suspend_64.h
index 7ebf0eb..60941de 100644
--- a/arch/x86/include/asm/suspend_64.h
+++ b/arch/x86/include/asm/suspend_64.h
@@ -24,6 +24,7 @@ struct saved_context {
 	unsigned long cr0, cr2, cr3, cr4, cr8;
 	u64 misc_enable;
 	bool misc_enable_saved;
+	struct msr_context msr_to_save;
 	unsigned long efer;
 	u16 gdt_pad; /* Unused */
 	struct desc_ptr gdt_desc;
diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 9ab5279..fd3243a 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -23,6 +23,7 @@
 #include <asm/debugreg.h>
 #include <asm/cpu.h>
 #include <asm/mmu_context.h>
+#include <linux/dmi.h>
 
 #ifdef CONFIG_X86_32
 __visible unsigned long saved_context_ebx;
@@ -32,6 +33,29 @@ __visible unsigned long saved_context_eflags;
 #endif
 struct saved_context saved_context;
 
+static void msr_save_context(struct saved_context *ctxt)
+{
+	struct msr_save_data *msr = ctxt->msr_to_save.msr_array;
+	struct msr_save_data *end = msr + ctxt->msr_to_save.num;
+
+	while (msr < end) {
+		msr->msr_saved = !rdmsrl_safe(msr->rv.msr_no, &msr->rv.reg.q);
+		msr++;
+	}
+}
+
+static void msr_restore_context(struct saved_context *ctxt)
+{
+	struct msr_save_data *msr = ctxt->msr_to_save.msr_array;
+	struct msr_save_data *end = msr + ctxt->msr_to_save.num;
+
+	while (msr < end) {
+		if (msr->msr_saved)
+			wrmsrl(msr->rv.msr_no, msr->rv.reg.q);
+		msr++;
+	}
+}
+
 /**
  *	__save_processor_state - save CPU registers before creating a
  *		hibernation image and before restoring the memory state from it
@@ -111,6 +135,7 @@ static void __save_processor_state(struct saved_context *ctxt)
 #endif
 	ctxt->misc_enable_saved = !rdmsrl_safe(MSR_IA32_MISC_ENABLE,
 					       &ctxt->misc_enable);
+	msr_save_context(ctxt);
 }
 
 /* Needed by apm.c */
@@ -229,6 +254,7 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	x86_platform.restore_sched_clock_state();
 	mtrr_bp_restore();
 	perf_restore_debug_store();
+	msr_restore_context(ctxt);
 }
 
 /* Needed by apm.c */
@@ -320,3 +346,75 @@ static int __init bsp_pm_check_init(void)
 }
 
 core_initcall(bsp_pm_check_init);
+
+/*
+ * The following section is a quirk framework for problematic BIOS:
+ * Sometimes MSRs are modified by BIOS after suspended to
+ * RAM, this might cause unexpected behavior after wakeup.
+ * Thus we save/restore these specified MSRs during suspending
+ * in order to work around it.
+ * A typical bug is reported at:
+ * https://bugzilla.redhat.com/show_bug.cgi?id=1227208
+ */
+static int msr_init_context(const u32 *msr_id, const int total_num)
+{
+	int i = 0;
+	struct msr_save_data *msr_data = NULL;
+
+	if (saved_context.msr_to_save.msr_array ||
+			saved_context.msr_to_save.num > 0) {
+		pr_err("PM: quirk already applied, please check your dmi match table.\n");
+		return -EINVAL;
+	}
+
+	msr_data = kmalloc_array(total_num,
+			sizeof(struct msr_save_data), GFP_KERNEL);
+	if (!msr_data) {
+		pr_err("PM: can not allocate memory to save/restore MSRs during suspend.\n");
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < total_num; i++) {
+		msr_data[i].rv.msr_no = msr_id[i];
+		msr_data[i].msr_saved = false;
+		msr_data[i].rv.reg.q = 0;
+	}
+	saved_context.msr_to_save.num = total_num;
+	saved_context.msr_to_save.msr_array = msr_data;
+	return 0;
+}
+
+/*
+ * For any further problematic BIOS/platforms,
+ * please add your own function similar to msr_initialize_bdw.
+ */
+static int msr_initialize_bdw(const struct dmi_system_id *d)
+{
+	/* Add any extra MSR ids into this array. */
+	u32 bdw_msr_id[] = {MSR_IA32_THERM_CONTROL};
+
+	pr_info("PM: %s detected, MSR saving is needed during suspending.\n",
+		d->ident);
+	return msr_init_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
+}
+
+static struct dmi_system_id msr_save_dmi_table[] = {
+	{
+	 .callback = msr_initialize_bdw,
+	 .ident = "BROADWELL BDX_EP",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "GRANTLEY"),
+		DMI_MATCH(DMI_PRODUCT_VERSION, "E63448-400"),
+		},
+	},
+	{}
+};
+
+static int pm_check_save_msr(void)
+{
+	dmi_check_system(msr_save_dmi_table);
+	return 0;
+}
+
+late_initcall(pm_check_save_msr);
-- 
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1244902 — Re: [PATCH][v5] x86, suspend: Save/restore extra MSR registers for suspend

FromBorislav Petkov <bp@suse.de>
Date2015-10-12 18:40 +0200
SubjectRe: [PATCH][v5] x86, suspend: Save/restore extra MSR registers for suspend
Message-ID<qiOiK-4vG-25@gated-at.bofh.it>
In reply to#1244053
On Sun, Oct 11, 2015 at 11:20:01AM +0800, Chen Yu wrote:
> A bug is reported(https://bugzilla.redhat.com/show_bug.cgi?id=1227208)

I get:

"You are not authorized to access bug #1227208.

Most likely the bug has been restricted for internal development
processes and we cannot grant access."

> that, after resumed from S3, CPU is running at a low speed.
> After investigation, it is found that, BIOS has modified the value
> of THERM_CONTROL register during S3, and changes it from 0 to 0x10
> (thus changes the clock modulation from reserved to enabled),
> since value of 0x10 means CPU can only get 25% of the Duty Cycle,
> this triggers the problem.

Is this what the bug described above is? In any case, please remove the
private bugzilla link and describe the bug in text here.

Also, from reading the other thread about the v4 patch, it sounds like
intel_pstate can't handle the clock modulation properly, according to
what Doug says.

So let's have this aspect sorted out properly first please before adding
yet another ugly BIOS workaround.

Btw, why can't that BIOS be fixed instead?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245012

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2015-10-12 20:50 +0200
Message-ID<qiQkz-7rT-59@gated-at.bofh.it>
In reply to#1244902
On Monday, October 12, 2015 06:37:50 PM Borislav Petkov wrote:
> On Sun, Oct 11, 2015 at 11:20:01AM +0800, Chen Yu wrote:
> > A bug is reported(https://bugzilla.redhat.com/show_bug.cgi?id=1227208)
> 
> I get:
> 
> "You are not authorized to access bug #1227208.
> 
> Most likely the bug has been restricted for internal development
> processes and we cannot grant access."
> 
> > that, after resumed from S3, CPU is running at a low speed.
> > After investigation, it is found that, BIOS has modified the value
> > of THERM_CONTROL register during S3, and changes it from 0 to 0x10
> > (thus changes the clock modulation from reserved to enabled),
> > since value of 0x10 means CPU can only get 25% of the Duty Cycle,
> > this triggers the problem.
> 
> Is this what the bug described above is? In any case, please remove the
> private bugzilla link and describe the bug in text here.
> 
> Also, from reading the other thread about the v4 patch, it sounds like
> intel_pstate can't handle the clock modulation properly, according to
> what Doug says.
> 
> So let's have this aspect sorted out properly first please before adding
> yet another ugly BIOS workaround.
> 
> Btw, why can't that BIOS be fixed instead?

Because it's been shipped to users.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1247031 — RE: [PATCH][v5] x86, suspend: Save/restore extra MSR registers for suspend

From"Chen, Yu C" <yu.c.chen@intel.com>
Date2015-10-14 19:50 +0200
SubjectRE: [PATCH][v5] x86, suspend: Save/restore extra MSR registers for suspend
Message-ID<qjylA-5Hm-3@gated-at.bofh.it>
In reply to#1244902
SGksIEJvcmlzLCB0aGFua3MgZm9yIHlvdSByZXBseQ0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2Fn
ZS0tLS0tDQo+IEZyb206IEJvcmlzbGF2IFBldGtvdiBbbWFpbHRvOmJwQHN1c2UuZGVdDQo+IFNl
bnQ6IFR1ZXNkYXksIE9jdG9iZXIgMTMsIDIwMTUgMTI6MzggQU0NCj4gVG86IENoZW4sIFl1IEMN
Cj4gQ2M6IG1pbmdvQHJlZGhhdC5jb207IHJqd0Byand5c29ja2kubmV0OyBwYXZlbEB1Y3cuY3o7
DQo+IHRnbHhAbGludXRyb25peC5kZTsgaHBhQHp5dG9yLmNvbTsgWmhhbmcsIFJ1aTsgbHV0b0Br
ZXJuZWwub3JnOw0KPiBsaW51eEBob3Jpem9uLmNvbTsgZHNteXRoaWVzQHRlbHVzLm5ldDsgbGlu
dXgtcG1Admdlci5rZXJuZWwub3JnOw0KPiB4ODZAa2VybmVsLm9yZzsgbGludXgta2VybmVsQHZn
ZXIua2VybmVsLm9yZzsgS2FzemV3c2tpLCBNYXJjaW4NCj4gU3ViamVjdDogUmU6IFtQQVRDSF1b
djVdIHg4Niwgc3VzcGVuZDogU2F2ZS9yZXN0b3JlIGV4dHJhIE1TUiByZWdpc3RlcnMgZm9yDQo+
IHN1c3BlbmQNCj4gDQo+IE9uIFN1biwgT2N0IDExLCAyMDE1IGF0IDExOjIwOjAxQU0gKzA4MDAs
IENoZW4gWXUgd3JvdGU6DQo+ID4gQSBidWcgaXMgcmVwb3J0ZWQoaHR0cHM6Ly9idWd6aWxsYS5y
ZWRoYXQuY29tL3Nob3dfYnVnLmNnaT9pZD0xMjI3MjA4KQ0KPiANCj4gSSBnZXQ6DQo+IA0KPiAi
WW91IGFyZSBub3QgYXV0aG9yaXplZCB0byBhY2Nlc3MgYnVnICMxMjI3MjA4Lg0KPiANCj4gTW9z
dCBsaWtlbHkgdGhlIGJ1ZyBoYXMgYmVlbiByZXN0cmljdGVkIGZvciBpbnRlcm5hbCBkZXZlbG9w
bWVudCBwcm9jZXNzZXMNCj4gYW5kIHdlIGNhbm5vdCBncmFudCBhY2Nlc3MuIg0KPiANCj4gPiB0
aGF0LCBhZnRlciByZXN1bWVkIGZyb20gUzMsIENQVSBpcyBydW5uaW5nIGF0IGEgbG93IHNwZWVk
Lg0KPiA+IEFmdGVyIGludmVzdGlnYXRpb24sIGl0IGlzIGZvdW5kIHRoYXQsIEJJT1MgaGFzIG1v
ZGlmaWVkIHRoZSB2YWx1ZSBvZg0KPiA+IFRIRVJNX0NPTlRST0wgcmVnaXN0ZXIgZHVyaW5nIFMz
LCBhbmQgY2hhbmdlcyBpdCBmcm9tIDAgdG8gMHgxMCAodGh1cw0KPiA+IGNoYW5nZXMgdGhlIGNs
b2NrIG1vZHVsYXRpb24gZnJvbSByZXNlcnZlZCB0byBlbmFibGVkKSwgc2luY2UgdmFsdWUgb2YN
Cj4gPiAweDEwIG1lYW5zIENQVSBjYW4gb25seSBnZXQgMjUlIG9mIHRoZSBEdXR5IEN5Y2xlLCB0
aGlzIHRyaWdnZXJzIHRoZQ0KPiA+IHByb2JsZW0uDQo+IA0KPiBJcyB0aGlzIHdoYXQgdGhlIGJ1
ZyBkZXNjcmliZWQgYWJvdmUgaXM/IEluIGFueSBjYXNlLCBwbGVhc2UgcmVtb3ZlIHRoZQ0KPiBw
cml2YXRlIGJ1Z3ppbGxhIGxpbmsgYW5kIGRlc2NyaWJlIHRoZSBidWcgaW4gdGV4dCBoZXJlLg0K
T0ssIHdpbGwgcmVtb3ZlIHRoZSBsaW5rIGhlcmUgYW5kIGluc2lkZSB0aGUgY29kZS4NCj4gDQo+
IEFsc28sIGZyb20gcmVhZGluZyB0aGUgb3RoZXIgdGhyZWFkIGFib3V0IHRoZSB2NCBwYXRjaCwg
aXQgc291bmRzIGxpa2UNCj4gaW50ZWxfcHN0YXRlIGNhbid0IGhhbmRsZSB0aGUgY2xvY2sgbW9k
dWxhdGlvbiBwcm9wZXJseSwgYWNjb3JkaW5nIHRvIHdoYXQNCj4gRG91ZyBzYXlzLg0KPiANCj4g
U28gbGV0J3MgaGF2ZSB0aGlzIGFzcGVjdCBzb3J0ZWQgb3V0IHByb3Blcmx5IGZpcnN0IHBsZWFz
ZSBiZWZvcmUgYWRkaW5nIHlldA0KPiBhbm90aGVyIHVnbHkgQklPUyB3b3JrYXJvdW5kLg0KPiAN
CldlbGwsIHRoYXQgbWlnaHQgYmUgYW5vdGhlciBkcml2ZXIgYnVnIElNTy4gSSdsbCBkaWcgaW50
byB0aGF0IG9uZS4NCg0KPiBCdHcsIHdoeSBjYW4ndCB0aGF0IEJJT1MgYmUgZml4ZWQgaW5zdGVh
ZD8NClRoaXMgYnVnIGlzIGZvdW5kIG9uIGEgcmVsZWFzZWQgcmVkaGF0IHNlcnZlciwgc28gSSB0
aGluayBpdCB3b3VsZCBiZQ0KaGFyZCB0byB1cGRhdGUgYWxsIHVzZXJzJyBtYWNoaW5lcy4gDQo+
IA0KPiAtLQ0KPiBSZWdhcmRzL0dydXNzLA0KPiAgICAgQm9yaXMuDQoNCkJlc3QgUmVnYXJkcywN
Cll1DQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245002

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2015-10-12 20:50 +0200
Message-ID<qiQky-7rT-27@gated-at.bofh.it>
In reply to#1244053
On Sunday, October 11, 2015 11:20:01 AM Chen Yu wrote:
> A bug is reported(https://bugzilla.redhat.com/show_bug.cgi?id=1227208)
> that, after resumed from S3, CPU is running at a low speed.
> After investigation, it is found that, BIOS has modified the value
> of THERM_CONTROL register during S3, and changes it from 0 to 0x10
> (thus changes the clock modulation from reserved to enabled),
> since value of 0x10 means CPU can only get 25% of the Duty Cycle,
> this triggers the problem.
> 
> Here is a simple scenario to reproduce the issue(only for above case):
> 1.Boot up the system
> 2.Get MSR with address 0x19a, it should be 0
> 3.Put the system into sleep, then wake it up
> 4.Get MSR with address 0x19a, it should be 0(actually it shows 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 introducing a framework
> to save/restore specified MSR registers(THERM_CONTROL in this case)
> for suspend/resume.
> 
> When user encounters a problematic platform and needs to protect the
> MSRs during suspending, he can simply add a quirk entry in
> msr_save_dmi_table, and customizes MSR registers inside the quirk
> callback, for example:
> 
> u32 msr_id_need_to_save[] = {MSR_ID0, MSR_ID1, MSR_ID2...};
> 
> and the quirk mechanism ensures that, once resumed from suspended,
> the MSRs indicated by these IDs will be restored to their original values
> before suspended.
> 
> Since both 64/32-bit kernels are affected, this patch covers 64/32-bit
> common code path. And because the MSRs specified by the user might not
> be available or readable in any situation, we use rdmsrl_safe to safely
> save these MSRs.
> 
> Tested-by: Marcin Kaszewski <marcin.kaszewski@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
> v5:
>  - Rename some structures and variables for better understanding.
>    Put the defination of struct saved_msr and struct msr_save_data to
>    header: arch/x86/include/asm/msr.h.
>    Re-implement the msr_save_context and msr_restore_context for better
>    maintaining.
>    Convert the msr_context_array to be dynamically allocated.
>    Fix some typos in code comments.
> v4:
>  - Revert v3 to v2, and fix some typos in changelog/comments. 
>    Use msr_info structure instead of msr_id + msr_value.
>    Adjust some codes for better readability.
> v3:
>  - Simplify the patch to only focus on THERM_CONTROL register.
>    This will make things 'just work'.
> v2:
>  - Cover both 64/32-bit common code path.
>    Use rdmsrl_safe to safely read MSR.
>    Introduce a quirk framework for save/restore specified MSR on different
>    platforms.
> ---
>  arch/x86/include/asm/msr.h        | 10 ++++
>  arch/x86/include/asm/suspend_32.h |  1 +
>  arch/x86/include/asm/suspend_64.h |  1 +
>  arch/x86/power/cpu.c              | 98 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 110 insertions(+)
> 
> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> index 77d8b28..5ae24ed 100644
> --- a/arch/x86/include/asm/msr.h
> +++ b/arch/x86/include/asm/msr.h
> @@ -32,6 +32,16 @@ struct msr_regs_info {
>  	int err;
>  };
>  
> +struct msr_save_data {
> +	bool msr_saved;
> +	struct msr_info rv;
> +};
> +
> +struct msr_context {
> +	unsigned short num;
> +	struct msr_save_data *msr_array;
> +};
> +
>  static inline unsigned long long native_read_tscp(unsigned int *aux)
>  {
>  	unsigned long low, high;
> diff --git a/arch/x86/include/asm/suspend_32.h b/arch/x86/include/asm/suspend_32.h
> index d1793f0..5057f65 100644
> --- a/arch/x86/include/asm/suspend_32.h
> +++ b/arch/x86/include/asm/suspend_32.h
> @@ -15,6 +15,7 @@ struct saved_context {
>  	unsigned long cr0, cr2, cr3, cr4;
>  	u64 misc_enable;
>  	bool misc_enable_saved;
> +	struct msr_context msr_to_save;
>  	struct desc_ptr gdt_desc;
>  	struct desc_ptr idt;
>  	u16 ldt;
> diff --git a/arch/x86/include/asm/suspend_64.h b/arch/x86/include/asm/suspend_64.h
> index 7ebf0eb..60941de 100644
> --- a/arch/x86/include/asm/suspend_64.h
> +++ b/arch/x86/include/asm/suspend_64.h
> @@ -24,6 +24,7 @@ struct saved_context {
>  	unsigned long cr0, cr2, cr3, cr4, cr8;
>  	u64 misc_enable;
>  	bool misc_enable_saved;
> +	struct msr_context msr_to_save;
>  	unsigned long efer;
>  	u16 gdt_pad; /* Unused */
>  	struct desc_ptr gdt_desc;
> diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
> index 9ab5279..fd3243a 100644
> --- a/arch/x86/power/cpu.c
> +++ b/arch/x86/power/cpu.c
> @@ -23,6 +23,7 @@
>  #include <asm/debugreg.h>
>  #include <asm/cpu.h>
>  #include <asm/mmu_context.h>
> +#include <linux/dmi.h>
>  
>  #ifdef CONFIG_X86_32
>  __visible unsigned long saved_context_ebx;
> @@ -32,6 +33,29 @@ __visible unsigned long saved_context_eflags;
>  #endif
>  struct saved_context saved_context;
>  
> +static void msr_save_context(struct saved_context *ctxt)
> +{
> +	struct msr_save_data *msr = ctxt->msr_to_save.msr_array;
> +	struct msr_save_data *end = msr + ctxt->msr_to_save.num;
> +
> +	while (msr < end) {
> +		msr->msr_saved = !rdmsrl_safe(msr->rv.msr_no, &msr->rv.reg.q);
> +		msr++;
> +	}
> +}
> +
> +static void msr_restore_context(struct saved_context *ctxt)
> +{
> +	struct msr_save_data *msr = ctxt->msr_to_save.msr_array;
> +	struct msr_save_data *end = msr + ctxt->msr_to_save.num;
> +
> +	while (msr < end) {
> +		if (msr->msr_saved)
> +			wrmsrl(msr->rv.msr_no, msr->rv.reg.q);
> +		msr++;
> +	}
> +}
> +
>  /**
>   *	__save_processor_state - save CPU registers before creating a
>   *		hibernation image and before restoring the memory state from it
> @@ -111,6 +135,7 @@ static void __save_processor_state(struct saved_context *ctxt)
>  #endif
>  	ctxt->misc_enable_saved = !rdmsrl_safe(MSR_IA32_MISC_ENABLE,
>  					       &ctxt->misc_enable);
> +	msr_save_context(ctxt);
>  }
>  
>  /* Needed by apm.c */
> @@ -229,6 +254,7 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
>  	x86_platform.restore_sched_clock_state();
>  	mtrr_bp_restore();
>  	perf_restore_debug_store();
> +	msr_restore_context(ctxt);
>  }
>  
>  /* Needed by apm.c */
> @@ -320,3 +346,75 @@ static int __init bsp_pm_check_init(void)
>  }
>  
>  core_initcall(bsp_pm_check_init);
> +
> +/*
> + * The following section is a quirk framework for problematic BIOS:
> + * Sometimes MSRs are modified by BIOS after suspended to
> + * RAM, this might cause unexpected behavior after wakeup.
> + * Thus we save/restore these specified MSRs during suspending
> + * in order to work around it.
> + * A typical bug is reported at:
> + * https://bugzilla.redhat.com/show_bug.cgi?id=1227208
> + */
> +static int msr_init_context(const u32 *msr_id, const int total_num)
> +{
> +	int i = 0;
> +	struct msr_save_data *msr_data = NULL;
> +
> +	if (saved_context.msr_to_save.msr_array ||
> +			saved_context.msr_to_save.num > 0) {
> +		pr_err("PM: quirk already applied, please check your dmi match table.\n");
> +		return -EINVAL;
> +	}
> +
> +	msr_data = kmalloc_array(total_num,
> +			sizeof(struct msr_save_data), GFP_KERNEL);
> +	if (!msr_data) {
> +		pr_err("PM: can not allocate memory to save/restore MSRs during suspend.\n");
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < total_num; i++) {
> +		msr_data[i].rv.msr_no = msr_id[i];
> +		msr_data[i].msr_saved = false;
> +		msr_data[i].rv.reg.q = 0;
> +	}
> +	saved_context.msr_to_save.num = total_num;
> +	saved_context.msr_to_save.msr_array = msr_data;
> +	return 0;
> +}
> +
> +/*
> + * For any further problematic BIOS/platforms,
> + * please add your own function similar to msr_initialize_bdw.
> + */
> +static int msr_initialize_bdw(const struct dmi_system_id *d)
> +{
> +	/* Add any extra MSR ids into this array. */
> +	u32 bdw_msr_id[] = {MSR_IA32_THERM_CONTROL};
> +
> +	pr_info("PM: %s detected, MSR saving is needed during suspending.\n",
> +		d->ident);
> +	return msr_init_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
> +}
> +
> +static struct dmi_system_id msr_save_dmi_table[] = {
> +	{
> +	 .callback = msr_initialize_bdw,
> +	 .ident = "BROADWELL BDX_EP",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "GRANTLEY"),
> +		DMI_MATCH(DMI_PRODUCT_VERSION, "E63448-400"),
> +		},
> +	},
> +	{}
> +};
> +
> +static int pm_check_save_msr(void)
> +{
> +	dmi_check_system(msr_save_dmi_table);
> +	return 0;
> +}
> +
> +late_initcall(pm_check_save_msr);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web