linux-kernel - Re: [PATCH] [v2] x86, suspend: Save/restore extra MSR registers for suspend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150823062033.GB18134@gmail.com>
Date:	Sun, 23 Aug 2015 08:20:33 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Pavel Machek <pavel@....cz>
Cc:	Chen Yu <yu.c.chen@...el.com>, rjw@...ysocki.net,
	tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
	rui.zhang@...el.com, lenb@...nel.org, x86@...nel.org,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [v2] x86, suspend: Save/restore extra MSR registers for
 suspend


* Pavel Machek <pavel@....cz> wrote:

> On Fri 2015-08-21 19:53:34, Chen Yu 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 introducing a framework
> > for saving/restoring specify MSR registers(THERM_CONTROL in this case)
> > on suspend/resume.
> > 
> > When user finds a problematic platform that requires save/restore MSRs,
> > he can simply add quirk in msr_save_dmi_table, and customizes MSR
> > registers in quirk callback, for example:
> > 
> > unsigned int msr_id_need_to_save[] = {MSR_ID0, MSR_ID1, MSR_ID2...};
> > 
> > and system ensures that, once resumed from suspend, these MSR indicated
> > by IDs will be restored to their original values before suspend.
> > 
> > Since both 64/32-bit kernels are affected, this patch covers 64/32-bit
> > common code path. And because the MSR ids specified by user might not be
> > available or readable in any situation, we use rdmsrl_safe to safely
> > save these MSR registers.
> > 
> > Tested-by: Marcin Kaszewski <marcin.kaszewski@...el.com>
> > Signed-off-by: Chen Yu <yu.c.chen@...el.com>
> 
> Acked-by: Pavel Machek <pavel@....cz>

So I like the general structure of the patch and I like the MSR saving mechanism 
it introduces, but it is full of typos and small stylistic uncleanlinesses which 
need to be fixed before this patch can be applied. Please don't ack incomplete 
patches.

Thanks,

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