Messages in this thread | | | Subject | Re: [Xen-devel] [PATCH v4 0/5] [PATCH v3 0/5] Improve non-"safe" MSR access failure handling | From | Boris Ostrovsky <> | Date | Mon, 14 Mar 2016 10:32:13 -0400 |
| |
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
| |