Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

* 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. > > This is IMO awful: it papers over bugs. In particular,

Re: [Xen-devel] [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On 17/09/15 00:33, 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. > > This is IMO awful: it papers over bugs. In particular, KVM

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On Thu, Sep 17, 2015 at 09:19:20AM +0200, Ingo Molnar wrote: > Most big distro kernels on bare metal have CONFIG_PARAVIRT=y (I checked > Ubuntu and > Fedora), so we are potentially exposing a lot of users to problems. + SUSE. > Crashing the bootup on an unknown MSR is bad. Many MSR reads and

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On 17/09/2015 10:58, Peter Zijlstra wrote: > But the far greater problem I have with the whole virt thing is that > you cannot use rdmsr_safe() to probe if an MSR exists at all because, as > you told me, these virt thingies return 0 for all 'unknown' MSRs instead > of faulting. At least for

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On 17/09/2015 11:31, Borislav Petkov wrote: > >> > Crashing the bootup on an unknown MSR is bad. Many MSR reads and writes >> > are >> > non-critical and returning the 'safe' result is much better than crashing >> > or >> > hanging the bootup. > ... and prepending all MSR accesses with

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

However, the difference between one CONFIG and another is quite frankly crazy. We should explicitly use the safe versions where this is appropriate, and then yes, we should do this. Yet another reason the paravirt code is batshit crazy. On September 17, 2015 2:31:34 AM PDT, Borislav Petkov

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On Wed, Sep 16, 2015 at 04:33:11PM -0700, 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. > > This is IMO awful: it papers over

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On Thu, Sep 17, 2015 at 01:40:30PM +0200, Paolo Bonzini wrote: > > > On 17/09/2015 10:58, Peter Zijlstra wrote: > > But the far greater problem I have with the whole virt thing is that > > you cannot use rdmsr_safe() to probe if an MSR exists at all because, as > > you told me, these virt

Re: [Xen-devel] [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On 09/17/2015 05:10 AM, Andrew Cooper wrote: On 17/09/15 00:33, 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. This is IMO awful: it

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On Thu, Sep 17, 2015 at 12:19 AM, Ingo Molnar wrote: > > * 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

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On Thu, Sep 17, 2015 at 08:17:18AM -0700, Andy Lutomirski wrote: > > Ah, that would be good news. Andy earlier argued I could not rely on > > rdmsr_safe() faulting on unknown MSRs. If practically we can there's > > some code I can simplify :-) > > I was taking about QEMU TCG, not KVM. Just for

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On Thu, Sep 17, 2015 at 8:17 AM, Peter Zijlstra wrote: > On Thu, Sep 17, 2015 at 08:17:18AM -0700, Andy Lutomirski wrote: > >> > Ah, that would be good news. Andy earlier argued I could not rely on >> > rdmsr_safe() faulting on unknown MSRs. If practically we can there's >>

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On 17/09/2015 17:27, Arjan van de Ven wrote: >> >>> ( We should double check that rdmsr()/wrmsr() results are never left >>>uninitialized, but are set to zero or so, for cases where the >>> return code is not >>>checked. ) >> >> It sure looks like native_read_msr_safe doesn't clear the

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On 17/09/2015 17:26, Andy Lutomirski wrote: > Maybe Paolo can fix QEMU to fail bad MSR accesses for real... I was afraid of someone proposing exactly that. :) I can do it since the list of MSRs can be lifted from KVM. Let's first see the direction these patches go... Paolo -- To unsubscribe

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

( We should double check that rdmsr()/wrmsr() results are never left uninitialized, but are set to zero or so, for cases where the return code is not checked. ) It sure looks like native_read_msr_safe doesn't clear the output if the rdmsr fails. I'd suggest to return some poison not

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On 9/17/2015 8:29 AM, Paolo Bonzini wrote: On 17/09/2015 17:27, Arjan van de Ven wrote: ( We should double check that rdmsr()/wrmsr() results are never left uninitialized, but are set to zero or so, for cases where the return code is not checked. ) It sure looks like

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On Thu, Sep 17, 2015 at 01:39:26PM +0200, Paolo Bonzini wrote: > That's not a big deal, that's what *_safe is for. The problem is that > there are definitely some cases where the *_safe version is not being used. I mean to do feature checks which assure you that those MSRs are there so you don't

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On 17/09/2015 17:31, Arjan van de Ven wrote: >> >> What about 0 + WARN? > > why 0? > > 0xdeadbeef or any other pattern (even 0x3636363636) makes more sense (of > course also WARN... but most folks don't read dmesg for WARNs) > > (it's the same thing we do for list or slab poison stuff)

Re: [Xen-devel] [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On 17/09/15 16:27, Borislav Petkov wrote: > On Thu, Sep 17, 2015 at 01:39:26PM +0200, Paolo Bonzini wrote: >> That's not a big deal, that's what *_safe is for. The problem is that >> there are definitely some cases where the *_safe version is not being used. > I mean to do feature checks which

Re: [Xen-devel] [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On Thu, Sep 17, 2015 at 04:32:53PM +0100, Andrew Cooper wrote: > There are plenty of non-architectural MSRs in use which don't have > feature bits. That's exactly what the "possible" adjective was supposed to represent. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply.

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On Sep 17, 2015 5:33 AM, "Peter Zijlstra" wrote: > > On Thu, Sep 17, 2015 at 01:40:30PM +0200, Paolo Bonzini wrote: > > > > > > On 17/09/2015 10:58, Peter Zijlstra wrote: > > > But the far greater problem I have with the whole virt thing is that > > > you cannot use

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

* Andy Lutomirski wrote: > On Thu, Sep 17, 2015 at 12:19 AM, Ingo Molnar wrote: > > > > * Andy Lutomirski wrote: > > > >> Setting CONFIG_PARAVIRT=y has an unintended side effect: it silently > >> turns all rdmsr and wrmsr operations into

Re: [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

On Thu, Sep 17, 2015 at 10:30 AM, Ingo Molnar wrote: > > * Andy Lutomirski wrote: > >> On Thu, Sep 17, 2015 at 12:19 AM, Ingo Molnar wrote: >> > >> > * Andy Lutomirski wrote: >> > >> >> Setting CONFIG_PARAVIRT=y has an

[PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

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. This is IMO awful: it papers over bugs. In particular, KVM gueests might be unwittingly depending on this