KVM: X86: A few fixes around ignore_msrs

MSR accesses can be one of:

(1) KVM internal access,
(2) userspace access (e.g., via KVM_SET_MSRS ioctl),
(3) guest access.

The ignore_msrs was previously handled by kvm_get_msr_common() and kvm_set_msr_common(), which is the bottom of the msr access stack. It's working in most cases, however it could dump unwanted warning messages to dmesg even if kvm get/set the msrs internally when calling __kvm_set_msr() or __kvm_get_msr() (e.g. kvm_cpuid()). Ideally we only want to trap cases (2) or (3), but not (1) above.

To achieve this, move the ignore_msrs handling upper until the callers of __kvm_get_msr() and __kvm_set_msr(). To identify the "msr missing" event, a new return value (KVM_MSR_RET_INVALID==2) is used for that.

IMO, kvm_cpuid() is simply buggy. If KVM attempts to access a non-existent MSR then it darn well should warn.

arch/x86/kvm/cpuid.c +0 -0

index 8a294f9747aa..7ef7283011d6 100644

Quoted 16 lines · Sean Christopherson

IMO, kvm_cpuid() is simply buggy. If KVM attempts to access a non-existent MSR then it darn well should warn.

arch/x86/kvm/cpuid.c +2 -1

index 8a294f9747aa..7ef7283011d6 100644
@@ -1013,7 +1013,8 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
                *ebx = entry->ebx;
                *ecx = entry->ecx;
                *edx = entry->edx;
-               if (function == 7 && index == 0) {
+               if (function == 7 && index == 0 && (*ebx | (F(RTM) | F(HLE))) &&
+                   (vcpu->arch.arch_capabilities & ARCH_CAP_TSX_CTRL_MSR)) {
                        u64 data;
                        if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&

arch/x86/kvm/cpuid.c bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx, · line 1020

Quoted 2 lines

arch/x86/kvm/cpuid.c +0 -0

                            (data & TSX_CTRL_CPUID_CLEAR))

That works too, but I disagree that warning is the correct behavior here. It certainly should warn as long as kvm_get_msr blindly returns zero. However, for a guest it's fine to access a potentially non-existent MSR if you're ready to trap the #GP, and the point of this series is to let cpuid.c or any other KVM code do the same.

Quoted 17 lines · Paolo Bonzini

Quoted 16 lines · Sean Christopherson

IMO, kvm_cpuid() is simply buggy. If KVM attempts to access a non-existent MSR then it darn well should warn.

arch/x86/kvm/cpuid.c +2 -1

index 8a294f9747aa..7ef7283011d6 100644
@@ -1013,7 +1013,8 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
                *ebx = entry->ebx;
                *ecx = entry->ecx;
                *edx = entry->edx;
-               if (function == 7 && index == 0) {
+               if (function == 7 && index == 0 && (*ebx | (F(RTM) | F(HLE))) &&
+                   (vcpu->arch.arch_capabilities & ARCH_CAP_TSX_CTRL_MSR)) {
                        u64 data;
                        if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&

Quoted 8 lines

arch/x86/kvm/cpuid.c bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx, · line 1019

Quoted 2 lines

arch/x86/kvm/cpuid.c +0 -0

                            (data & TSX_CTRL_CPUID_CLEAR))

That works too, but I disagree that warning is the correct behavior here. It certainly should warn as long as kvm_get_msr blindly returns zero. However, for a guest it's fine to access a potentially non-existent MSR if you're ready to trap the #GP, and the point of this series is to let cpuid.c or any other KVM code do the same.

I get the "what" of the change, and even the "why" to some extent, but I dislike the idea of supporting/encouraging blind reads/writes to MSRs. Blind writes are just asking for problems, and suppressing warnings on reads is almost guaranteed to be suppressing a KVM bug.

Case in point, looking at the TSX thing again, I actually think the fix should be:

arch/x86/kvm/cpuid.c +2 -2

index 5eb618dbf211..64322446e590 100644
@@ -1013,9 +1013,9 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
                *ebx = entry->ebx;
                *ecx = entry->ecx;
                *edx = entry->edx;
-               if (function == 7 && index == 0) {
+               if (function == 7 && index == 0 && (*ebx | (F(RTM) | F(HLE))) {
                        u64 data;
-                       if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&
+                       if (!kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data) &&
                            (data & TSX_CTRL_CPUID_CLEAR))
                                *ebx &= ~(F(RTM) | F(HLE));
                }

On VMX, MSR_IA32_TSX_CTRL will be added to the so called shared MSR array regardless of whether or not it is being advertised to userspace (this is a bug in its own right). Using the host_initiated variant means KVM will incorrectly bypass VMX's ARCH_CAP_TSX_CTRL_MSR check, i.e. incorrectly clear the bits if userspace is being weird and stuffed MSR_IA32_TSX_CTRL without advertising it to the guest.

In short, the whole MSR_IA32_TSX_CTRL implementation seems messy and this is just papering over that mess. The correct fix is to invoke setup_msrs() on writes to MSR_IA32_ARCH_CAPABILITIES, filtering MSR_IA32_TSX_CTRL out of shared MSRs when it's not advertised, and change kvm_cpuid() to use the unpriveleged variant.

TSC_CTRL aside, if we insist on pointing a gun at our foot at some point, this should be a dedicated flavor of MSR access, e.g. msr_data.kvm_initiated, so that it at least requires intentionally loading the gun.

I get the "what" of the change, and even the "why" to some extent, but I dislike the idea of supporting/encouraging blind reads/writes to MSRs. Blind writes are just asking for problems, and suppressing warnings on reads is almost guaranteed to be suppressing a KVM bug.

TSC_CTRL aside, if we insist on pointing a gun at our foot at some point, this should be a dedicated flavor of MSR access, e.g. msr_data.kvm_initiated, so that it at least requires intentionally loading the gun.

Quoted 4 lines · Sean Christopherson

I get the "what" of the change, and even the "why" to some extent, but I dislike the idea of supporting/encouraging blind reads/writes to MSRs. Blind writes are just asking for problems, and suppressing warnings on reads is almost guaranteed to be suppressing a KVM bug.

Right, that's why this patch does not just suppress warnings: it adds a different return value to detect the case.

Quoted 3 lines

TSC_CTRL aside, if we insist on pointing a gun at our foot at some point, this should be a dedicated flavor of MSR access, e.g. msr_data.kvm_initiated, so that it at least requires intentionally loading the gun.

With this patch, __kvm_get_msr does not know about ignore_msrs at all, that seems to be strictly an improvement; do you agree with that?

What would you think about adding warn_unused_result to __kvm_get_msr?

Not really? It's solving a problem that doesn't exist in the current code base (assuming TSC_CTRL is fixed), and IMO solving it in an ugly fashion.

I would much prefer that, _if_ we want to support blind KVM-internal MSR accesses, we end up with code like:

	if (msr_info->kvm_internal) {
		return 1;
	} else if (!ignore_msrs) {
		vcpu_debug_ratelimited(vcpu, "unhandled wrmsr: 0x%x data 0x%llx\n",
			    msr, data);
		return 1;
	} else {
		if (report_ignored_msrs)
			vcpu_unimpl(vcpu,
				"ignored wrmsr: 0x%x data 0x%llx\n",
				msr, data);
		break;
	}

But I'm still not convinced that there is a legimiate scenario for setting kvm_internal=true.

Quoted 20 lines · Sean Christopherson

Not really? It's solving a problem that doesn't exist in the current code base (assuming TSC_CTRL is fixed), and IMO solving it in an ugly fashion.

I would much prefer that, _if_ we want to support blind KVM-internal MSR accesses, we end up with code like:

	if (msr_info->kvm_internal) {
		return 1;
	} else if (!ignore_msrs) {
		vcpu_debug_ratelimited(vcpu, "unhandled wrmsr: 0x%x data 0x%llx\n",
			    msr, data);
		return 1;
	} else {
		if (report_ignored_msrs)
			vcpu_unimpl(vcpu,
				"ignored wrmsr: 0x%x data 0x%llx\n",
				msr, data);
		break;
	}

Quoted 2 lines

But I'm still not convinced that there is a legimiate scenario for setting kvm_internal=true.

Actually this really looks like my initial version when I was discussing this with Paolo before this version, but Paolo suggested what I implemented last. I think I agree with Paolo that it's an improvement to have a way to get/set real msr value so that we don't need to further think about effects being taken with the two tricky msr knobs (report_ignored_msrs, ignore_msrs). These knobs are even trickier to me when they're hidden deep, because they are not easily expected when seeing the name of the functions (e.g. __kvm_set_msr, rather than __kvm_set_msr_retval_fixed).

arch/x86/kvm/cpuid.c +2 -2

index 5eb618dbf211..64322446e590 100644
@@ -1013,9 +1013,9 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
                *ebx = entry->ebx;
                *ecx = entry->ecx;
                *edx = entry->edx;
-               if (function == 7 && index == 0) {
+               if (function == 7 && index == 0 && (*ebx | (F(RTM) | F(HLE))) {
                        u64 data;
-                       if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&
+                       if (!kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data) &&
                            (data & TSX_CTRL_CPUID_CLEAR))
                                *ebx &= ~(F(RTM) | F(HLE));
                }

On VMX, MSR_IA32_TSX_CTRL will be added to the so called shared MSR array regardless of whether or not it is being advertised to userspace (this is a bug in its own right). Using the host_initiated variant means KVM will incorrectly bypass VMX's ARCH_CAP_TSX_CTRL_MSR check, i.e. incorrectly clear the bits if userspace is being weird and stuffed MSR_IA32_TSX_CTRL without advertising it to the guest.

Quoted 18 lines · Sean Christopherson

arch/x86/kvm/cpuid.c +2 -2

index 5eb618dbf211..64322446e590 100644
@@ -1013,9 +1013,9 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
                *ebx = entry->ebx;
                *ecx = entry->ecx;
                *edx = entry->edx;
-               if (function == 7 && index == 0) {
+               if (function == 7 && index == 0 && (*ebx | (F(RTM) | F(HLE))) {
                        u64 data;
-                       if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&
+                       if (!kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data) &&
                            (data & TSX_CTRL_CPUID_CLEAR))
                                *ebx &= ~(F(RTM) | F(HLE));
                }

Quoted 6 lines

On VMX, MSR_IA32_TSX_CTRL will be added to the so called shared MSR array regardless of whether or not it is being advertised to userspace (this is a bug in its own right). Using the host_initiated variant means KVM will incorrectly bypass VMX's ARCH_CAP_TSX_CTRL_MSR check, i.e. incorrectly clear the bits if userspace is being weird and stuffed MSR_IA32_TSX_CTRL without advertising it to the guest.

Btw, would it be more staightforward to check "vcpu->arch.arch_capabilities & ARCH_CAP_TSX_CTRL_MSR" rather than "*ebx | (F(RTM) | F(HLE))" even if we want to have such a fix?

Quoted 19 lines · Peter Xu

Quoted 18 lines · Sean Christopherson

arch/x86/kvm/cpuid.c +2 -2

index 5eb618dbf211..64322446e590 100644
@@ -1013,9 +1013,9 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
                *ebx = entry->ebx;
                *ecx = entry->ecx;
                *edx = entry->edx;
-               if (function == 7 && index == 0) {
+               if (function == 7 && index == 0 && (*ebx | (F(RTM) | F(HLE))) {
                        u64 data;
-                       if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&
+                       if (!kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data) &&
                            (data & TSX_CTRL_CPUID_CLEAR))
                                *ebx &= ~(F(RTM) | F(HLE));
                }

Quoted 10 lines

Quoted 6 lines

On VMX, MSR_IA32_TSX_CTRL will be added to the so called shared MSR array regardless of whether or not it is being advertised to userspace (this is a bug in its own right). Using the host_initiated variant means KVM will incorrectly bypass VMX's ARCH_CAP_TSX_CTRL_MSR check, i.e. incorrectly clear the bits if userspace is being weird and stuffed MSR_IA32_TSX_CTRL without advertising it to the guest.

Btw, would it be more staightforward to check "vcpu->arch.arch_capabilities & ARCH_CAP_TSX_CTRL_MSR" rather than "*ebx | (F(RTM) | F(HLE))" even if we want to have such a fix?

Not really, That ends up duplicating the check in vmx_get_msr(). From an emulation perspective, this really is a "guest" access to the MSR, in the sense that it the virtual CPU is in the guest domain, i.e. not a god-like entity that gets to break the rules of emulation.

Quoted 3 lines

Btw, would it be more staightforward to check "vcpu->arch.arch_capabilities & ARCH_CAP_TSX_CTRL_MSR" rather than "*ebx | (F(RTM) | F(HLE))" even if we want to have such a fix?

Not really, That ends up duplicating the check in vmx_get_msr(). From an emulation perspective, this really is a "guest" access to the MSR, in the sense that it the virtual CPU is in the guest domain, i.e. not a god-like entity that gets to break the rules of emulation.

Quoted 7 lines · Sean Christopherson

Quoted 3 lines

Btw, would it be more staightforward to check "vcpu->arch.arch_capabilities & ARCH_CAP_TSX_CTRL_MSR" rather than "*ebx | (F(RTM) | F(HLE))" even if we want to have such a fix?

Not really, That ends up duplicating the check in vmx_get_msr(). From an emulation perspective, this really is a "guest" access to the MSR, in the sense that it the virtual CPU is in the guest domain, i.e. not a god-like entity that gets to break the rules of emulation.

But if you wrote a guest that wants to read MSR_IA32_TSX_CTRL, there are two choices:

1) check ARCH_CAPABILITIES first

2) blindly access it and default to 0.

Both are fine, because we know MSR_IA32_TSX_CTRL has no reserved/must-be-one bits. Calling __kvm_get_msr and checking for an invalid MSR through the return value is not breaking the rules of emulation, it is "faking" a #GP handler.

So I think Peter's patch is fine, but (possibly on top as a third patch) __must_check should be added to MSR getters and setters. Also one possibility is to return -EINVAL for invalid MSRs.

Paolo

Quoted 12 lines · Paolo Bonzini

Quoted 7 lines · Sean Christopherson

Quoted 3 lines

Btw, would it be more staightforward to check "vcpu->arch.arch_capabilities & ARCH_CAP_TSX_CTRL_MSR" rather than "*ebx | (F(RTM) | F(HLE))" even if we want to have such a fix?

Not really, That ends up duplicating the check in vmx_get_msr(). From an emulation perspective, this really is a "guest" access to the MSR, in the sense that it the virtual CPU is in the guest domain, i.e. not a god-like entity that gets to break the rules of emulation.

But if you wrote a guest that wants to read MSR_IA32_TSX_CTRL, there are two choices:

Quoted 8 lines

1) check ARCH_CAPABILITIES first

2) blindly access it and default to 0.

Both are fine, because we know MSR_IA32_TSX_CTRL has no reserved/must-be-one bits. Calling __kvm_get_msr and checking for an invalid MSR through the return value is not breaking the rules of emulation, it is "faking" a #GP handler.

"guest" was the wrong choice of word. My point was that, IMO, emulation should never set host_initiated=true.

To me, accessing MSRs with host_initiated is the equivalent of loading a ucode patch, i.e. it's super duper special stuff that deliberately turns off all safeguards and can change the fundamental behavior of the (virtual) CPU.

Quoted 4 lines

So I think Peter's patch is fine, but (possibly on top as a third patch) __must_check should be added to MSR getters and setters. Also one possibility is to return -EINVAL for invalid MSRs.

Quoted 3 lines

Quoted 3 lines

So I think Peter's patch is fine, but (possibly on top as a third patch) __must_check should be added to MSR getters and setters. Also one possibility is to return -EINVAL for invalid MSRs.

Yeah I can add another patch for that. Also if to repost, I tend to also introduce KVM_MSR_RET_[OK|ERROR] too, which seems to be cleaner when we had KVM_MSR_RET_INVALID.

Quoted 6 lines · Peter Xu

Quoted 3 lines

Quoted 3 lines

So I think Peter's patch is fine, but (possibly on top as a third patch) __must_check should be added to MSR getters and setters. Also one possibility is to return -EINVAL for invalid MSRs.

Yeah I can add another patch for that. Also if to repost, I tend to also introduce KVM_MSR_RET_[OK|ERROR] too, which seems to be cleaner when we had KVM_MSR_RET_INVALID.

You can use either that or 0/1/-EINVAL. I would be fine with that to keep the patch small.

Quoted 22 lines · Sean Christopherson

Quoted 12 lines · Paolo Bonzini

Quoted 7 lines · Sean Christopherson

Quoted 3 lines

Btw, would it be more staightforward to check "vcpu->arch.arch_capabilities & ARCH_CAP_TSX_CTRL_MSR" rather than "*ebx | (F(RTM) | F(HLE))" even if we want to have such a fix?

Not really, That ends up duplicating the check in vmx_get_msr(). From an emulation perspective, this really is a "guest" access to the MSR, in the sense that it the virtual CPU is in the guest domain, i.e. not a god-like entity that gets to break the rules of emulation.

But if you wrote a guest that wants to read MSR_IA32_TSX_CTRL, there are two choices:

Quoted 8 lines

1) check ARCH_CAPABILITIES first

2) blindly access it and default to 0.

Both are fine, because we know MSR_IA32_TSX_CTRL has no reserved/must-be-one bits. Calling __kvm_get_msr and checking for an invalid MSR through the return value is not breaking the rules of emulation, it is "faking" a #GP handler.

Quoted 7 lines

"guest" was the wrong choice of word. My point was that, IMO, emulation should never set host_initiated=true.

To me, accessing MSRs with host_initiated is the equivalent of loading a ucode patch, i.e. it's super duper special stuff that deliberately turns off all safeguards and can change the fundamental behavior of the (virtual) CPU.

This seems to be an orthogonal change against what this series tried to do. We use host_initiated=true in current code, and this series won't change that fact either. As I mentioned in the other thread, at least the rdmsr warning is ambiguous when it's not initiated from the guest if without this patchset, and this series could address that.

Quoted 3 lines

Quoted 3 lines

So I think Peter's patch is fine, but (possibly on top as a third patch) __must_check should be added to MSR getters and setters. Also one possibility is to return -EINVAL for invalid MSRs.

Yeah I can add another patch for that. Also if to repost, I tend to also introduce KVM_MSR_RET_[OK|ERROR] too, which seems to be cleaner when we had KVM_MSR_RET_INVALID.

Any objections before I repost?

Quoted 31 lines · Peter Xu

Quoted 22 lines · Sean Christopherson

Quoted 12 lines · Paolo Bonzini

Quoted 7 lines · Sean Christopherson

Quoted 3 lines

Btw, would it be more staightforward to check "vcpu->arch.arch_capabilities & ARCH_CAP_TSX_CTRL_MSR" rather than "*ebx | (F(RTM) | F(HLE))" even if we want to have such a fix?

Not really, That ends up duplicating the check in vmx_get_msr(). From an emulation perspective, this really is a "guest" access to the MSR, in the sense that it the virtual CPU is in the guest domain, i.e. not a god-like entity that gets to break the rules of emulation.

But if you wrote a guest that wants to read MSR_IA32_TSX_CTRL, there are two choices:

Quoted 8 lines

1) check ARCH_CAPABILITIES first

2) blindly access it and default to 0.

Both are fine, because we know MSR_IA32_TSX_CTRL has no reserved/must-be-one bits. Calling __kvm_get_msr and checking for an invalid MSR through the return value is not breaking the rules of emulation, it is "faking" a #GP handler.

Quoted 7 lines

"guest" was the wrong choice of word. My point was that, IMO, emulation should never set host_initiated=true.

To me, accessing MSRs with host_initiated is the equivalent of loading a ucode patch, i.e. it's super duper special stuff that deliberately turns off all safeguards and can change the fundamental behavior of the (virtual) CPU.

Quoted 5 lines

This seems to be an orthogonal change against what this series tried to do. We use host_initiated=true in current code, and this series won't change that fact either. As I mentioned in the other thread, at least the rdmsr warning is ambiguous when it's not initiated from the guest if without this patchset, and this series could address that.

My argument is that using host_initiated=true is wrong.

Quoted 4 lines

Quoted 3 lines

Quoted 3 lines

So I think Peter's patch is fine, but (possibly on top as a third patch) __must_check should be added to MSR getters and setters. Also one possibility is to return -EINVAL for invalid MSRs.

Quoted 5 lines

Yeah I can add another patch for that. Also if to repost, I tend to also introduce KVM_MSR_RET_[OK|ERROR] too, which seems to be cleaner when we had KVM_MSR_RET_INVALID.

Any objections before I repost?

Heh, or perhaps "Any objections that haven't been overruled before I repost?" :-D

Again, using host_initiated or not should be a different issue? Frankly speaking, I don't know whether it's an issue or not, but it's different from what this series wants to do, because it'll be the same before/after this series. Am I right?

Or, please explain what's the "overruled objection" that you're talking about..

Quoted 4 lines · Peter Xu

Again, using host_initiated or not should be a different issue? Frankly speaking, I don't know whether it's an issue or not, but it's different from what this series wants to do, because it'll be the same before/after this series. Am I right?

I'm arguing that the TSX thing should be

arch/x86/kvm/cpuid.c +1 -1

index 5eb618dbf211..e1fd5ac0df96 100644
@@ -1015,7 +1015,7 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
                *edx = entry->edx;
                if (function == 7 && index == 0) {
                        u64 data;
-                       if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&
+                       if (!kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data) &&
                            (data & TSX_CTRL_CPUID_CLEAR))
                                *ebx &= ~(F(RTM) | F(HLE));
                }

At which point hoisting the ignored message up a few levels is pointless because the only users of __kvm_*et_msr() will do the explicit ignored_check. And I'm also arguing that KVM should never use __kvm_get_msr() for its own actions, as host_initiated=true should only be used for host VMM accesses and host_initiated=false actions should go through the proper checks and never get to the ignored_msrs logic (assuming no KVM bug).

Quoted 1 line

Or, please explain what's the "overruled objection" that you're talking about..

Sean: Objection your honor.
Paolo: Overruled, you're wrong.
Sean: Phooey.

My point is that even though I still object to this series, Paolo has final say.

Quoted 6 lines

Sean: Objection your honor.
Paolo: Overruled, you're wrong.
Sean: Phooey.

My point is that even though I still object to this series, Paolo has final say.

I could be wrong, but I feel like Paolo was really respecting your input, as always.

It's just as simple as a 2:1 vote, isn't it? (I can still count myself in for the vote, right? :)

Btw, you didn't reply to my other email:

https://lore.kernel.org/kvm/20200626191118.GC175520@xz-x1/

Let me change the question a bit - Do you think e.g. we should never use rdmsr*_safe() in the Linux kernel as long as the MSR has a bit somewhere telling whether the MSR exists (so we should never trigger #GP on these MSRs)? I think it's a similar question that we're discussing here..

Quoted 4 lines · Peter Xu

Quoted 4 lines

Sean: Objection your honor.
Paolo: Overruled, you're wrong.
Sean: Phooey.

Quoted 5 lines

Quoted 2 lines

My point is that even though I still object to this series, Paolo has final say.

I could be wrong, but I feel like Paolo was really respecting your input, as always.

I do respect Sean's input

but I also believe that in this case there's three questions:

a) should KVM be allowed to use the equivalent of rdmsr*_safe() on guest MSRs? I say a mild yes, Sean says a strong no.

b) is it good to separate the "1" and "-EINVAL" results so that ignore_msrs handling can be moved out of the MSR access functions? I say yes because KVM should never rely on ignore_msrs; Sean didn't say anything (it's not too relevant if you answer no to the first question).

c) is it possible to reimplement TSX_CTRL_MSR to avoid using the equivalent of rdmsr*_safe()? Sean says yes and it's not really possible to argue against that, but then it doesn't really matter if you answer yes to the first two questions.

Sean sees your patch mostly as answering "yes" to the question (a), and therefore disagrees with it. I see your patch mostly as answering "yes" to question (b), and therefore like it. I would also accept a patch that reimplements TSX_CTRL_MSR (question c), but I consider your patch to be an improvement anyway (question b).

Quoted 2 lines

It's just as simple as a 2:1 vote, isn't it? (I can still count myself in for the vote, right? :)

I do have the final say but I try to use that as little as possible (or never). And then it happens that ever so rare disagreements cluster in the same week!

The important thing is to analyze the source of the disagreement. Usually when that happens, it's because a change has multiple purposes and people see it in a different way.

In this case, I'm happy to accept this patch (and overrule Sean) not because he's wrong on question (a), but because in my opinion the actual motivation of the patch is question (b).

To be fair, I would prefer it if ignore_msrs didn't apply to host-initiated MSR accesses at all (only guest accesses). That would make this series much much simpler. It wouldn't solve the disagremement on question (a), but perhaps it would be a patch that Sean would agree on.