Re: [Xen-devel] [PATCH v4 0/5] [PATCH v3 0/5] Improve non-"safe" MSR access failure handling

[Boris Ostrovsky]

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