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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

Since there's plenty of time before the next merge window, I think we should apply and fix anything that breaks.

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.

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.

Quoted 1 line · Ingo Molnar

Most big distro kernels on bare metal have CONFIG_PARAVIRT=y (I

checked Ubuntu and

Quoted 1 line

Fedora), so we are potentially exposing a lot of users to problems.

+ SUSE.

Quoted 1 line

Crashing the bootup on an unknown MSR is bad. Many MSR reads and

writes are

Quoted 1 line

non-critical and returning the 'safe' result is much better than

crashing or

Quoted 1 line

hanging the bootup.

... and prepending all MSR accesses with feature/CPUID checks is probably almost
impossible.

Quoted 1 line · Ingo Molnar

Most big distro kernels on bare metal have CONFIG_PARAVIRT=y (I

checked Ubuntu and

Quoted 1 line

Fedora), so we are potentially exposing a lot of users to problems.

+ SUSE.

Quoted 1 line

Crashing the bootup on an unknown MSR is bad. Many MSR reads and

writes are

Quoted 1 line

non-critical and returning the 'safe' result is much better than

crashing or

Quoted 1 line

hanging the bootup.

... and prepending all MSR accesses with feature/CPUID checks is probably almost
impossible.

Quoted 4 lines

Quoted 4 lines

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 feature/CPUID checks is probably almost impossible.

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.

Quoted 2 lines · Paolo Bonzini

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 need the safe variants. And that is not always easy/possible.

There are plenty of non-architectural MSRs in use which don't have feature bits.

There are plenty of non-architectural MSRs in use which don't have feature bits.

Quoted 2 lines · Paolo Bonzini

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 need the safe variants. And that is not always easy/possible.

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.

Quoted 4 lines

Quoted 4 lines

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 feature/CPUID checks is probably almost impossible.

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.

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.

Quoted 10 lines · Andy Lutomirski

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Quoted 6 lines

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

Since there's plenty of time before the next merge window, I think we should apply and fix anything that breaks.

No, I think we should at most generate a warning instead, and not crash the kernel via rdmsr()!

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.

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.

( 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. )

Quoted 10 lines · Andy Lutomirski

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Quoted 6 lines

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

Since there's plenty of time before the next merge window, I think we should apply and fix anything that breaks.

No, I think we should at most generate a warning instead, and not crash the kernel via rdmsr()!

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.

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.

( 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. )

Quoted 4 lines

( 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.

Quoted 8 lines

Quoted 5 lines

( 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 just 0...

less likely to get interesting surprises that are insane hard to debug/diagnose

Quoted 6 lines · Arjan van de Ven

Quoted 6 lines

Quoted 5 lines

( 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. )

Quoted 4 lines

Quoted 2 lines

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 just 0...

What about 0 + WARN?

Quoted 2 lines

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)

Quoted 2 lines

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)

Quoted 6 lines · Arjan van de Ven

Quoted 6 lines

Quoted 5 lines

( 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. )

Quoted 4 lines

Quoted 2 lines

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 just 0...

What about 0 + WARN?

Quoted 8 lines

Quoted 5 lines

( 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 just 0...

less likely to get interesting surprises that are insane hard to debug/diagnose

Quoted 4 lines

( 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.

Quoted 26 lines · Ingo Molnar

Quoted 10 lines · Andy Lutomirski

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Quoted 6 lines

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

Since there's plenty of time before the next merge window, I think we should apply and fix anything that breaks.

No, I think we should at most generate a warning instead, and not crash the kernel via rdmsr()!

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.

Quoted 4 lines

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.

Should we do that for CONFIG_PARAVIRT=n, too?

It would be straightforward to rig this up (temporarily?) on top of these patches. To keep bloat down, we might want to implement it in do_general_protection rather than sticking it in native_read_msr.

wrmsr is a different beast, since we can fail due to writing the wrong value to an otherwise valid MSR. Given that MSR screwups can very easily be security holes, I'm not sure that warning and blindly continuing on an unchecked failed wrmsr is a good idea.

In any event, I think it's nuts that CONFIG_PARAVIRT changes this behavior. We should pick something sane and stick with it.

Quoted 3 lines

( 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.

Quoted 27 lines · Andy Lutomirski

Quoted 26 lines · Ingo Molnar

Quoted 10 lines · Andy Lutomirski

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Quoted 6 lines

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

Since there's plenty of time before the next merge window, I think we should apply and fix anything that breaks.

No, I think we should at most generate a warning instead, and not crash the kernel via rdmsr()!

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.

Quoted 6 lines

Quoted 4 lines

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.

Should we do that for CONFIG_PARAVIRT=n, too?

Absolutely. PARAVIRT=n should not behave differently from PARAVIRT=y on bare metal.

Quoted 3 lines

It would be straightforward to rig this up (temporarily?) on top of these patches. To keep bloat down, we might want to implement it in do_general_protection rather than sticking it in native_read_msr.

Fair enough.

Quoted 4 lines

wrmsr is a different beast, since we can fail due to writing the wrong value to an otherwise valid MSR. Given that MSR screwups can very easily be security holes, I'm not sure that warning and blindly continuing on an unchecked failed wrmsr is a good idea.

So the fact is that right now we are silently ignoring failures there, and have been doing that for some time. The right first step is to live with that and start generating low-key, once-per-bootup warnings at most, and see how frequent (and how serious) they are.

We could add a (default disabled) CONFIG_PANIC_ON_UNKNOWN_MSR=y option if that's really a serious concern.

Quoted 3 lines

In any event, I think it's nuts that CONFIG_PARAVIRT changes this behavior. We should pick something sane and stick with it.

Absolutely - and as it happens, the 'does not crash the kernel' PARAVIRT=y accidental behavior is actually quite close to what we wanted for a long time, so let's make it official - and add a warning to make sure we are aware of problems.

But don't turn 'potential problems' into showstopper bugs such as a hard to debug early boot crash, which to most Linux users means a black screen on bootup!

Quoted 27 lines · Andy Lutomirski

Quoted 26 lines · Ingo Molnar

Quoted 10 lines · Andy Lutomirski

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Quoted 6 lines

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

Since there's plenty of time before the next merge window, I think we should apply and fix anything that breaks.

No, I think we should at most generate a warning instead, and not crash the kernel via rdmsr()!

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.

Quoted 6 lines

Quoted 4 lines

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.

Should we do that for CONFIG_PARAVIRT=n, too?

Absolutely. PARAVIRT=n should not behave differently from PARAVIRT=y on bare metal.

Quoted 3 lines

It would be straightforward to rig this up (temporarily?) on top of these patches. To keep bloat down, we might want to implement it in do_general_protection rather than sticking it in native_read_msr.

Fair enough.

Quoted 4 lines

wrmsr is a different beast, since we can fail due to writing the wrong value to an otherwise valid MSR. Given that MSR screwups can very easily be security holes, I'm not sure that warning and blindly continuing on an unchecked failed wrmsr is a good idea.

So the fact is that right now we are silently ignoring failures there, and have been doing that for some time. The right first step is to live with that and start generating low-key, once-per-bootup warnings at most, and see how frequent (and how serious) they are.

We could add a (default disabled) CONFIG_PANIC_ON_UNKNOWN_MSR=y option if that's really a serious concern.

Quoted 3 lines

In any event, I think it's nuts that CONFIG_PARAVIRT changes this behavior. We should pick something sane and stick with it.

Absolutely - and as it happens, the 'does not crash the kernel' PARAVIRT=y accidental behavior is actually quite close to what we wanted for a long time, so let's make it official - and add a warning to make sure we are aware of problems.

But don't turn 'potential problems' into showstopper bugs such as a hard to debug early boot crash, which to most Linux users means a black screen on bootup!

Quoted 26 lines · Ingo Molnar

Quoted 10 lines · Andy Lutomirski

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Quoted 6 lines

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

Since there's plenty of time before the next merge window, I think we should apply and fix anything that breaks.

No, I think we should at most generate a warning instead, and not crash the kernel via rdmsr()!

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.

Quoted 4 lines

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.

Should we do that for CONFIG_PARAVIRT=n, too?

It would be straightforward to rig this up (temporarily?) on top of these patches. To keep bloat down, we might want to implement it in do_general_protection rather than sticking it in native_read_msr.

wrmsr is a different beast, since we can fail due to writing the wrong value to an otherwise valid MSR. Given that MSR screwups can very easily be security holes, I'm not sure that warning and blindly continuing on an unchecked failed wrmsr is a good idea.

In any event, I think it's nuts that CONFIG_PARAVIRT changes this behavior. We should pick something sane and stick with it.

Quoted 3 lines

( 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.

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

Since there's plenty of time before the next merge window, I think we should apply and fix anything that breaks.

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

Since there's plenty of time before the next merge window, I think we should apply and fix anything that breaks.

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 :)

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.

Quoted 4 lines · Peter Zijlstra

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 KVM, that behavior is opt-in (the ignore_msrs parameter) and no distro that I know enables it by default.

Quoted 8 lines · Paolo Bonzini

Quoted 4 lines · Peter Zijlstra

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.

Quoted 2 lines

At least for KVM, that behavior is opt-in (the ignore_msrs parameter) and no distro that I know enables it by default.

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 :-)

Quoted 3 lines

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.

Quoted 5 lines · Andy Lutomirski

Quoted 3 lines

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 my education, TCG is the thing without _any_ hardware assist?

The thing you fear to use because it cannot boot a kernel this side of tomorrow etc.. ?

Maybe Paolo can fix QEMU to fail bad MSR accesses for real...

Maybe Paolo can fix QEMU to fail bad MSR accesses for real...

Quoted 5 lines · Andy Lutomirski

Quoted 3 lines

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 my education, TCG is the thing without _any_ hardware assist?

The thing you fear to use because it cannot boot a kernel this side of tomorrow etc.. ?

Quoted 3 lines

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.

Quoted 8 lines · Paolo Bonzini

Quoted 4 lines · Peter Zijlstra

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.

Quoted 2 lines

At least for KVM, that behavior is opt-in (the ignore_msrs parameter) and no distro that I know enables it by default.

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 :-)

Quoted 4 lines · Peter Zijlstra

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 KVM, that behavior is opt-in (the ignore_msrs parameter) and no distro that I know enables it by default.

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.

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

Since there's plenty of time before the next merge window, I think we should apply and fix anything that breaks.

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 :)

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

Quoted 10 lines · Andy Lutomirski

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Quoted 3 lines

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

The Xen side of things need some further modification before this would be a safe operation to perform.

On the wrmsr side of things alone, this is the list of things Xen currently objects to and injects #GP faults for.

(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 00000000c0000081 from 0xe023e00800000000 to 0x0023001000000000.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 00000000c0000082 from 0xffff82d0bffff000 to 0xffffffff81560060.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 00000000c0000083 from 0xffff82d0bffff020 to 0xffffffff81558100.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 0000000000000174 from 0x000000000000e008 to 0x0000000000000010.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 0000000000000175 from 0xffff8300ac0f7fc0 to 0x0000000000000000.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 0000000000000176 from 0xffff82d08023fd50 to 0xffffffff815616d0.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 00000000c0000083 from 0xffff82d0bffff020 to 0xffffffff81561910.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 00000000c0000084 from 0x0000000000074700 to 0x0000000000047700.

However, it would be certainly be worth teaching PVops not to play with MSRs it doesn't own.

Quoted 10 lines · Andy Lutomirski

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Quoted 3 lines

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.

The Xen side of things need some further modification before this would be a safe operation to perform.

On the wrmsr side of things alone, this is the list of things Xen currently objects to and injects #GP faults for.

(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 00000000c0000081 from 0xe023e00800000000 to 0x0023001000000000.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 00000000c0000082 from 0xffff82d0bffff000 to 0xffffffff81560060.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 00000000c0000083 from 0xffff82d0bffff020 to 0xffffffff81558100.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 0000000000000174 from 0x000000000000e008 to 0x0000000000000010.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 0000000000000175 from 0xffff8300ac0f7fc0 to 0x0000000000000000.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 0000000000000176 from 0xffff82d08023fd50 to 0xffffffff815616d0.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 00000000c0000083 from 0xffff82d0bffff020 to 0xffffffff81561910.
(XEN) traps.c:2692:d0v0 Domain attempted WRMSR 00000000c0000084 from 0x0000000000074700 to 0x0000000000047700.

However, it would be certainly be worth teaching PVops not to play with MSRs it doesn't own.

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 behavior because CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT. I'm not aware of any such problems, but applying this series would be a good way to shake them out.

Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n and CONFIG_PARAVIRT=y as long as Xen isn't being used. The Xen maintainers are welcome to make a similar change on top of this.