From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Machek Subject: Re: [PATCH] x86 / hibernate: Use hlt_play_dead() when resuming from hibernation Date: Thu, 28 Jul 2016 21:33:40 +0200 Message-ID: <20160728193340.GC11657@amd> References: <1467105403-5085-1-git-send-email-yu.c.chen@intel.com> <12570565.xIMhLmhDgj@vostro.rjw.lan> <20160713095617.GB27870@amd> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]:48975 "EHLO atrey.karlin.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756905AbcG1Td6 (ORCPT ); Thu, 28 Jul 2016 15:33:58 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: "Rafael J. Wysocki" Cc: "Rafael J. Wysocki" , Linux PM , the arch/x86 maintainers , Chen Yu , Thomas Gleixner , "H. Peter Anvin" , Borislav Petkov , Peter Zijlstra , Ingo Molnar , Len Brown , Linux Kernel Mailing List , James Morse On Wed 2016-07-13 14:01:52, Rafael J. Wysocki wrote: > On Wed, Jul 13, 2016 at 11:56 AM, Pavel Machek wrote: > > On Sun 2016-07-10 03:49:25, Rafael J. Wysocki wrote: > >> From: Rafael J. Wysocki > >> > >> On Intel hardware, native_play_dead() uses mwait_play_dead() by > >> default and only falls back to the other methods if that fails. > >> That also happens during resume from hibernation, when the restore > >> (boot) kernel runs disable_nonboot_cpus() to take all of the CPUs > >> except for the boot one offline. > >> > >> However, that is problematic, because the address passed to > >> __monitor() in mwait_play_dead() is likely to be written to in the > >> last phase of hibernate image restoration and that causes the "dead" > >> CPU to start executing instructions again. Unfortunately, the page > >> containing the address in that CPU's instruction pointer may not be > >> valid any more at that point. > >> > >> First, that page may have been overwritten with image kernel memory > >> contents already, so the instructions the CPU attempts to execute may > >> simply be invalid. Second, the page tables previously used by that > >> CPU may have been overwritten by image kernel memory contents, so the > >> address in its instruction pointer is impossible to resolve then. > >> > >> A report from Varun Koyyalagunta and investigation carried out by > >> Chen Yu show that the latter sometimes happens in practice. > >> > >> To prevent it from happening, modify native_play_dead() to make > >> it use hlt_play_dead() instead of mwait_play_dead() during resume > >> from hibernation which avoids the inadvertent "revivals" of "dead" > >> CPUs. > >> > >> A slightly unpleasant consequence of this change is that if the > >> system is hibernated with one or more CPUs offline, it will generally > >> draw more power after resume than it did before hibernation, because > >> the physical state entered by CPUs via hlt_play_dead() is higher-power > >> than the mwait_play_dead() one in the majority of cases. It is > >> possible to work around this, but it is unclear how much of a problem > >> that's going to be in practice, so the workaround will be implemented > >> later if it turns out to be necessary. > >> > >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=106371 > >> Reported-by: Varun Koyyalagunta > >> Original-by: Chen Yu > >> Signed-off-by: Rafael J. Wysocki > > > > I notice that it changes even i386, where it should not be > > neccessary. But we probably should switch i386 to support similar to > > x86-64 one day (and I have patches) so no problem there. > > > > But I wonder if simpler solution is to place the mwait semaphore into > > known address? (Nosave region comes to mind?) > > It might work, but it wouldn't be simpler. > > First off, we'd need to monitor a separate cache line for each CPU > (see the message from Chen Yu) and it'd be a pain to guarantee that. > Second, CPUs may be woken up from MWAIT for other reasons, so that > needs to be taken into account too. > > In principle, we might set up a MONITOR?MWAIT "play dead" loop in a > safe page and make the "dead" CPUs jump to it during image restore, > but then the image kernel (after getting control back) would need to > migrate them away from there again, so doing the "halt" thing is *way* > simpler than that. Ok, it looks you have the best solution. Thanks... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html