Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934423AbcCNOdR (ORCPT ); Mon, 14 Mar 2016 10:33:17 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:38801 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932705AbcCNOdO (ORCPT ); Mon, 14 Mar 2016 10:33:14 -0400 Subject: Re: [Xen-devel] [PATCH v4 0/5] [PATCH v3 0/5] Improve non-"safe" MSR access failure handling To: Andy Lutomirski , X86 ML References: Cc: KVM list , Peter Zijlstra , Linus Torvalds , "linux-kernel@vger.kernel.org" , xen-devel , Borislav Petkov , Paolo Bonzini , Andrew Morton , Arjan van de Ven From: Boris Ostrovsky Message-ID: <56E6CB6D.5080906@oracle.com> Date: Mon, 14 Mar 2016 10:32:13 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2951 Lines: 66 On 03/12/2016 01:08 PM, Andy Lutomirski wrote: > Setting CONFIG_PARAVIRT=y has an unintended side effect: it silently > turns all rdmsr and wrmsr operations into the safe variants without > any checks that the operations actually succeed. > > With CONFIG_PARAVIRT=n, unchecked MSR failures OOPS and probably > cause boot to fail if they happen before init starts. > > Neither behavior is very good, and it's particularly unfortunate that > the behavior changes depending on CONFIG_PARAVIRT. > > In particular, KVM guests might be unwittingly depending on the > PARAVIRT=y behavior because CONFIG_KVM_GUEST currently depends on > CONFIG_PARAVIRT, and, because accesses in that case are completely > unchecked, we wouldn't even see a warning. > > This series changes the native behavior, regardless of > CONFIG_PARAVIRT. A non-"safe" MSR failure will give an informative > warning once and will be fixed up -- native_read_msr will return > zero, and both reads and writes will continue where they left off. > > If panic_on_oops is set, they will still OOPS and panic. > > By using the shiny new custom exception handler infrastructure, > there should be no overhead on the success paths. > > I didn't change the behavior on Xen, but, with this series applied, > it would be straightforward for the Xen maintainers to make the > corresponding change -- knowledge of whether the access is "safe" is > now propagated into the pvops. > > Doing this is probably a prerequisite to sanely decoupling > CONFIG_KVM_GUEST and CONFIG_PARAVIRT, which would probably make > Arjan and the rest of the Clear Containers people happy :) > > There's also room to reduce the code size of the "safe" variants > using custom exception handlers in the future. > > Changes from v3: > - WARN_ONCE instead of WARN (Ingo) > - In the warning text, s/unsafe/unchecked/ (Ingo, sort of) > > Changes from earlier versions: lots of changes! > > Andy Lutomirski (5): > x86/paravirt: Add _safe to the read_msr and write_msr PV hooks > x86/msr: Carry on after a non-"safe" MSR access fails without > !panic_on_oops > x86/paravirt: Add paravirt_{read,write}_msr > x86/paravirt: Make "unsafe" MSR accesses unsafe even if PARAVIRT=y > x86/msr: Set the return value to zero when native_rdmsr_safe fails > > arch/x86/include/asm/msr.h | 20 ++++++++++++---- > arch/x86/include/asm/paravirt.h | 45 +++++++++++++++++++++-------------- > arch/x86/include/asm/paravirt_types.h | 14 +++++++---- > arch/x86/kernel/paravirt.c | 6 +++-- > arch/x86/mm/extable.c | 33 +++++++++++++++++++++++++ > arch/x86/xen/enlighten.c | 27 +++++++++++++++++++-- > 6 files changed, 114 insertions(+), 31 deletions(-) > I don't see any issues as far as Xen is concerned but let me run this through our nightly. I'll wait for the next version (which I think you might have based on the comments). Please copy me. -boris