x86/msr: MSR access failure changes

This demotes an OOPS and likely panic due to a failed non-"safe" MSR access to a WARN_ON_ONCE and a return of zero (in the RDMSR case). We still write a pr_info entry unconditionally for debugging.

This demotes an OOPS and likely panic due to a failed non-"safe" MSR access to a WARN_ON_ONCE and a return of zero (in the RDMSR case). We still write a pr_info entry unconditionally for debugging.

Quoted 3 lines · Andy Lutomirski

This demotes an OOPS and likely panic due to a failed non-"safe" MSR access to a WARN_ON_ONCE and a return of zero (in the RDMSR case). We still write a pr_info entry unconditionally for debugging.

No, this is wrong.

If you really want to do something like this, then just make all MSR reads safe. So the only difference between "safe" and "unsafe" is that the unsafe version just doesn't check the return value, and silently just returns zero for reads (or writes nothing).

To quote Obi-Wan: "Use the exception table, Luke".

Because decoding instructions is just too ugly. We'll do it for CPU errata where we might have to do it for user space code too (ie the AMD prefetch mess), but for code that _we_ control? Hell no.

So NAK on this.

Quoted 13 lines · Linus Torvalds

Quoted 3 lines · Andy Lutomirski

This demotes an OOPS and likely panic due to a failed non-"safe" MSR access to a WARN_ON_ONCE and a return of zero (in the RDMSR case). We still write a pr_info entry unconditionally for debugging.

No, this is wrong.

If you really want to do something like this, then just make all MSR reads safe. So the only difference between "safe" and "unsafe" is that the unsafe version just doesn't check the return value, and silently just returns zero for reads (or writes nothing).

Quoted 7 lines

To quote Obi-Wan: "Use the exception table, Luke".

Because decoding instructions is just too ugly. We'll do it for CPU errata where we might have to do it for user space code too (ie the AMD prefetch mess), but for code that _we_ control? Hell no.

So NAK on this.

My personal preference is to just not do this at all. A couple people disagree. If we make the unsafe variants not oops, then I think we want to have the nice loud warning, since these issues are bugs if they happen.

We could certainly use the exception table for this, but it'll result in bigger core, since each MSR access will need an exception table entry and an associated fixup to call some helper that warns and sets the result to zero.

I'd be happy to implement that, but only if it'll be applied. Otherwise I'd rather just drop this patch and keep the rest of the series.

Or we could extend exception table entry encoding to include a 'warning bit', to not bloat the kernel. If the exception handler code encounters such an exception it would generate a one-time warning for that entry, but otherwise not crash the kernel and continue execution with an all-zeroes result for the MSR read.

Or we could extend exception table entry encoding to include a 'warning bit', to not bloat the kernel. If the exception handler code encounters such an exception it would generate a one-time warning for that entry, but otherwise not crash the kernel and continue execution with an all-zeroes result for the MSR read.

Linus, what's your preference?

Quoted 2 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

    unsigned long long native_read_msr(unsigned int msr)
    {
       int err;
       unsigned long long val;

       val = native_read_msr_safe(msr, &err);
       WARN_ON_ONCE(err);
       return val;
    }

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

How many msr reads are <i>so</i> critical that the function call overhead would matter?

Quoted 3 lines

How many msr reads are <i>so</i> critical that the function call overhead would matter?

if anything qualifies it'd be switch_to() and friends.

note that I'm not entirely happy about the notion of "safe" MSRs. They're safe as in "won't fault".

Quoted 3 lines

How many msr reads are <i>so</i> critical that the function call overhead would matter?

if anything qualifies it'd be switch_to() and friends.

note that I'm not entirely happy about the notion of "safe" MSRs. They're safe as in "won't fault".

Quoted 20 lines · Linus Torvalds

Quoted 3 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

    unsigned long long native_read_msr(unsigned int msr)
    {
       int err;
       unsigned long long val;

       val = native_read_msr_safe(msr, &err);
       WARN_ON_ONCE(err);
       return val;
    }

Quoted 5 lines

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

How many msr reads are <i>so</i> critical that the function call overhead would matter?

if anything qualifies it'd be switch_to() and friends.

note that I'm not entirely happy about the notion of "safe" MSRs. They're safe as in "won't fault".
Reading random MSRs isn't a generic safe operation though, but the name sort of gives people
the impression that it is. Even with _safe variants, you still need to KNOW the MSR exists (by means
of CPUID or similar) unfortunately.

And maybe the KVM user return notifier.

And maybe the KVM user return notifier.

Quoted 20 lines · Linus Torvalds

Quoted 3 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

    unsigned long long native_read_msr(unsigned int msr)
    {
       int err;
       unsigned long long val;

       val = native_read_msr_safe(msr, &err);
       WARN_ON_ONCE(err);
       return val;
    }

Quoted 5 lines

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

How many msr reads are <i>so</i> critical that the function call overhead would matter?

if anything qualifies it'd be switch_to() and friends.

note that I'm not entirely happy about the notion of "safe" MSRs. They're safe as in "won't fault".
Reading random MSRs isn't a generic safe operation though, but the name sort of gives people
the impression that it is. Even with _safe variants, you still need to KNOW the MSR exists (by means
of CPUID or similar) unfortunately.

Quoted 2 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

    unsigned long long native_read_msr(unsigned int msr)
    {
       int err;
       unsigned long long val;

       val = native_read_msr_safe(msr, &err);
       WARN_ON_ONCE(err);
       return val;
    }

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

How many msr reads are <i>so</i> critical that the function call overhead would matter?

Quoted 2 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

   unsigned long long native_read_msr(unsigned int msr)
   {
      int err;
      unsigned long long val;

      val = native_read_msr_safe(msr, &err);
      WARN_ON_ONCE(err);
      return val;
   }

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

In the interest of sanity, I want to drop the "native_", too, since there appear to be few or no good use cases for native_read_msr as such. I'm tempted to add new functions read_msr and write_msr that forward to rdmsrl_safe and wrmsrl_safe.

It looks like the msr helpers are every bit as bad as the TSC helpers used to be :(

In the interest of sanity, I want to drop the "native_", too, since there appear to be few or no good use cases for native_read_msr as such. I'm tempted to add new functions read_msr and write_msr that forward to rdmsrl_safe and wrmsrl_safe.

It looks like the msr helpers are every bit as bad as the TSC helpers used to be :(

In the interest of sanity, I want to drop the "native_", too

In the interest of sanity, I want to drop the "native_", too

Quoted 2 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

   unsigned long long native_read_msr(unsigned int msr)
   {
      int err;
      unsigned long long val;

      val = native_read_msr_safe(msr, &err);
      WARN_ON_ONCE(err);
      return val;
   }

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

Quoted 2 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

   unsigned long long native_read_msr(unsigned int msr)
   {
      int err;
      unsigned long long val;

      val = native_read_msr_safe(msr, &err);
      WARN_ON_ONCE(err);
      return val;
   }

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

How many msr reads are <i>so</i> critical that the function call overhead would matter? Get rid of the inline version of the _safe() thing too, and put that thing there too.

Quoted 2 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

   unsigned long long native_read_msr(unsigned int msr)
   {
      int err;
      unsigned long long val;

      val = native_read_msr_safe(msr, &err);
      WARN_ON_ONCE(err);
      return val;
   }

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

How many msr reads are <i>so</i> critical that the function call overhead would matter? Get rid of the inline version of the _safe() thing too, and put that thing there too.

Quoted 2 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

   unsigned long long native_read_msr(unsigned int msr)
   {
      int err;
      unsigned long long val;

      val = native_read_msr_safe(msr, &err);
      WARN_ON_ONCE(err);
      return val;
   }

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

How many msr reads are <i>so</i> critical that the function call overhead would matter? Get rid of the inline version of the _safe() thing too, and put that thing there too.

Quoted 20 lines · Linus Torvalds

Quoted 2 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

   unsigned long long native_read_msr(unsigned int msr)
   {
      int err;
      unsigned long long val;

      val = native_read_msr_safe(msr, &err);
      WARN_ON_ONCE(err);
      return val;
   }

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

Quoted 3 lines

How many msr reads are <i>so</i> critical that the function call overhead would matter? Get rid of the inline version of the _safe() thing too, and put that thing there too.

There are a few in the perf code, and esp. on cores without a stack engine the call overhead is noticeable. Also note that the perf MSRs are generally optimized MSRs and less slow (we cannot say fast, they're still MSRs) than regular MSRs.

Quoted 20 lines · Linus Torvalds

Quoted 2 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

   unsigned long long native_read_msr(unsigned int msr)
   {
      int err;
      unsigned long long val;

      val = native_read_msr_safe(msr, &err);
      WARN_ON_ONCE(err);
      return val;
   }

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

Quoted 3 lines

How many msr reads are <i>so</i> critical that the function call overhead would matter? Get rid of the inline version of the _safe() thing too, and put that thing there too.

There are a few in the perf code, and esp. on cores without a stack engine the call overhead is noticeable. Also note that the perf MSRs are generally optimized MSRs and less slow (we cannot say fast, they're still MSRs) than regular MSRs.

Quoted 25 lines · Peter Zijlstra

Quoted 23 lines · Linus Torvalds

Quoted 2 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

   unsigned long long native_read_msr(unsigned int msr)
   {
      int err;
      unsigned long long val;

      val = native_read_msr_safe(msr, &err);
      WARN_ON_ONCE(err);
      return val;
   }

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

How many msr reads are <i>so</i> critical that the function call overhead would matter? Get rid of the inline version of the _safe() thing too, and put that thing there too.

Quoted 4 lines

There are a few in the perf code, and esp. on cores without a stack engine the call overhead is noticeable. Also note that the perf MSRs are generally optimized MSRs and less slow (we cannot say fast, they're still MSRs) than regular MSRs.

These could still be open coded in an inlined fashion, like the scheduler usage.

Thanks,

Ingo

Quoted 1 line

These could still be open coded in an inlined fashion, like the scheduler usage.

We could have a raw_rdmsr for those.

OTOH, I'm still not 100% convinced that this warn-but-don't-die behavior is worth the effort. This isn't a frequent source of bugs to my knowledge, and we don't try to recover from incorrect cr writes, out-of-bounds MMIO, etc, so do we really gain much by rigging a recovery mechanism for rdmsr and wrmsr failures for code that doesn't use the _safe variants?

Quoted 1 line

These could still be open coded in an inlined fashion, like the scheduler usage.

We could have a raw_rdmsr for those.

OTOH, I'm still not 100% convinced that this warn-but-don't-die behavior is worth the effort. This isn't a frequent source of bugs to my knowledge, and we don't try to recover from incorrect cr writes, out-of-bounds MMIO, etc, so do we really gain much by rigging a recovery mechanism for rdmsr and wrmsr failures for code that doesn't use the _safe variants?

Quoted 9 lines · Andy Lutomirski

Quoted 1 line

These could still be open coded in an inlined fashion, like the scheduler usage.

We could have a raw_rdmsr for those.

OTOH, I'm still not 100% convinced that this warn-but-don't-die behavior is worth the effort. This isn't a frequent source of bugs to my knowledge, and we don't try to recover from incorrect cr writes, out-of-bounds MMIO, etc, so do we really gain much by rigging a recovery mechanism for rdmsr and wrmsr failures for code that doesn't use the _safe variants?

It's just the general principle really: don't crash the kernel on bootup. There's few things more user hostile than that.

Also, this would maintain the status quo: since we now (accidentally) don't crash the kernel on distro kernels (but silently and unsafely ignore the faulting instruction), we should not regress that behavior (by adding the chance to crash again), but improve upon it.

Thanks,

Ingo

Quoted 9 lines · Andy Lutomirski

Quoted 1 line

These could still be open coded in an inlined fashion, like the scheduler usage.

We could have a raw_rdmsr for those.

OTOH, I'm still not 100% convinced that this warn-but-don't-die behavior is worth the effort. This isn't a frequent source of bugs to my knowledge, and we don't try to recover from incorrect cr writes, out-of-bounds MMIO, etc, so do we really gain much by rigging a recovery mechanism for rdmsr and wrmsr failures for code that doesn't use the _safe variants?

It's just the general principle really: don't crash the kernel on bootup. There's few things more user hostile than that.

Also, this would maintain the status quo: since we now (accidentally) don't crash the kernel on distro kernels (but silently and unsafely ignore the faulting instruction), we should not regress that behavior (by adding the chance to crash again), but improve upon it.

Thanks,

Ingo

Quoted 13 lines · Ingo Molnar

Quoted 9 lines · Andy Lutomirski

Quoted 1 line

These could still be open coded in an inlined fashion, like the scheduler usage.

We could have a raw_rdmsr for those.

OTOH, I'm still not 100% convinced that this warn-but-don't-die behavior is worth the effort. This isn't a frequent source of bugs to my knowledge, and we don't try to recover from incorrect cr writes, out-of-bounds MMIO, etc, so do we really gain much by rigging a recovery mechanism for rdmsr and wrmsr failures for code that doesn't use the _safe variants?

Quoted 7 lines

It's just the general principle really: don't crash the kernel on bootup. There's few things more user hostile than that.

Also, this would maintain the status quo: since we now (accidentally) don't crash the kernel on distro kernels (but silently and unsafely ignore the faulting instruction), we should not regress that behavior (by adding the chance to crash again), but improve upon it.

Just a heads up: the extable improvements in tip:ras/core make it straightforward to get the best of all worlds: explicit failure handling (written in C!), no fast path overhead whatsoever, and no new garbage in the exception handlers.

Quoted 13 lines · Ingo Molnar

Quoted 9 lines · Andy Lutomirski

Quoted 1 line

These could still be open coded in an inlined fashion, like the scheduler usage.

We could have a raw_rdmsr for those.

OTOH, I'm still not 100% convinced that this warn-but-don't-die behavior is worth the effort. This isn't a frequent source of bugs to my knowledge, and we don't try to recover from incorrect cr writes, out-of-bounds MMIO, etc, so do we really gain much by rigging a recovery mechanism for rdmsr and wrmsr failures for code that doesn't use the _safe variants?

Quoted 7 lines

It's just the general principle really: don't crash the kernel on bootup. There's few things more user hostile than that.

Also, this would maintain the status quo: since we now (accidentally) don't crash the kernel on distro kernels (but silently and unsafely ignore the faulting instruction), we should not regress that behavior (by adding the chance to crash again), but improve upon it.

Just a heads up: the extable improvements in tip:ras/core make it straightforward to get the best of all worlds: explicit failure handling (written in C!), no fast path overhead whatsoever, and no new garbage in the exception handlers.

Quoted 25 lines · Peter Zijlstra

Quoted 23 lines · Linus Torvalds

Quoted 2 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

   unsigned long long native_read_msr(unsigned int msr)
   {
      int err;
      unsigned long long val;

      val = native_read_msr_safe(msr, &err);
      WARN_ON_ONCE(err);
      return val;
   }

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

How many msr reads are <i>so</i> critical that the function call overhead would matter? Get rid of the inline version of the _safe() thing too, and put that thing there too.

Quoted 4 lines

There are a few in the perf code, and esp. on cores without a stack engine the call overhead is noticeable. Also note that the perf MSRs are generally optimized MSRs and less slow (we cannot say fast, they're still MSRs) than regular MSRs.

These could still be open coded in an inlined fashion, like the scheduler usage.

Thanks,

Ingo

Quoted 2 lines · Ingo Molnar

Linus, what's your preference?

So quite frankly, is there any reason we don't just implement native_read_msr() as just

   unsigned long long native_read_msr(unsigned int msr)
   {
      int err;
      unsigned long long val;

      val = native_read_msr_safe(msr, &err);
      WARN_ON_ONCE(err);
      return val;
   }

Note: no inline, no nothing. Just put it in arch/x86/lib/msr.c, and be done with it. I don't see the downside.

How many msr reads are <i>so</i> critical that the function call overhead would matter? Get rid of the inline version of the _safe() thing too, and put that thing there too.

How many msr reads are <i>so</i> critical that the function call overhead would matter? Get rid of the inline version of the _safe() thing too, and put that thing there too.

How many msr reads are <i>so</i> critical that the function call overhead would matter? Get rid of the inline version of the _safe() thing too, and put that thing there too.

Linus, what's your preference?

Quoted 13 lines · Linus Torvalds

Quoted 3 lines · Andy Lutomirski

This demotes an OOPS and likely panic due to a failed non-"safe" MSR access to a WARN_ON_ONCE and a return of zero (in the RDMSR case). We still write a pr_info entry unconditionally for debugging.

No, this is wrong.

If you really want to do something like this, then just make all MSR reads safe. So the only difference between "safe" and "unsafe" is that the unsafe version just doesn't check the return value, and silently just returns zero for reads (or writes nothing).

Quoted 7 lines

To quote Obi-Wan: "Use the exception table, Luke".

Because decoding instructions is just too ugly. We'll do it for CPU errata where we might have to do it for user space code too (ie the AMD prefetch mess), but for code that _we_ control? Hell no.

So NAK on this.

My personal preference is to just not do this at all. A couple people disagree. If we make the unsafe variants not oops, then I think we want to have the nice loud warning, since these issues are bugs if they happen.

We could certainly use the exception table for this, but it'll result in bigger core, since each MSR access will need an exception table entry and an associated fixup to call some helper that warns and sets the result to zero.

I'd be happy to implement that, but only if it'll be applied. Otherwise I'd rather just drop this patch and keep the rest of the series.

Quoted 3 lines · Andy Lutomirski

This demotes an OOPS and likely panic due to a failed non-"safe" MSR access to a WARN_ON_ONCE and a return of zero (in the RDMSR case). We still write a pr_info entry unconditionally for debugging.

No, this is wrong.

If you really want to do something like this, then just make all MSR reads safe. So the only difference between "safe" and "unsafe" is that the unsafe version just doesn't check the return value, and silently just returns zero for reads (or writes nothing).

To quote Obi-Wan: "Use the exception table, Luke".

Because decoding instructions is just too ugly. We'll do it for CPU errata where we might have to do it for user space code too (ie the AMD prefetch mess), but for code that _we_ control? Hell no.

So NAK on this.