Skip to contentSkip to conversation

KVM: nVMX: Use SET_MSR_OR_WARN() to simplify failure logging

PATCH In review Oliver Upton 3 participants 4 messages Nov 28, 2019 – Dec 13, 2019

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Oliver Upton

author
#0 lore raw

PATCH 1/1 KVM: nVMX: Use SET_MSR_OR_WARN() to simplify failure logging

commit 458151f65b4d ("KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on VM-Exit") introduced the SET_MSR_OR_WARN() macro to WARN when kvm_set_msr() fails. Replace other occurences of this pattern with the macro to remove the need of printing on failure.

Signed-off-by Oliver Upton <[email protected]>
---
 arch/x86/kvm/vmx/nested.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)
arch/x86/kvm/vmx/nested.c +2 -10
index 4aea7d304beb..f7dbaac7cb90 100644
@@ -928,12 +928,8 @@ static u32 nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
 				__func__, i, e.index, e.reserved);
 			goto fail;
 		}
-		if (kvm_set_msr(vcpu, e.index, e.value)) {
-			pr_debug_ratelimited(
-				"%s cannot write MSR (%u, 0x%x, 0x%llx)\n",
-				__func__, i, e.index, e.value);
+		if (SET_MSR_OR_WARN(vcpu, e.index, e.value))
 			goto fail;
-		}
 	}
 	return 0;
 fail:

@@ -4175,12 +4171,8 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu)
 				goto vmabort;
 			}
 
-			if (kvm_set_msr(vcpu, h.index, h.value)) {
-				pr_debug_ratelimited(
-					"%s WRMSR failed (%u, 0x%x, 0x%llx)\n",
-					__func__, j, h.index, h.value);
+			if (SET_MSR_OR_WARN(vcpu, h.index, h.value))
 				goto vmabort;
-			}
 		}
 	}
Signature · 1 line
2.24.0.432.g9d3f5f5b63-goog
#1 lore raw
Quoted 29 lines · Oliver Upton

commit 458151f65b4d ("KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on VM-Exit") introduced the SET_MSR_OR_WARN() macro to WARN when kvm_set_msr() fails. Replace other occurences of this pattern with the macro to remove the need of printing on failure.

Signed-off-by Oliver Upton <[email protected]>
---
 arch/x86/kvm/vmx/nested.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)
arch/x86/kvm/vmx/nested.c +1 -5
index 4aea7d304beb..f7dbaac7cb90 100644
@@ -928,12 +928,8 @@ static u32 nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
 				__func__, i, e.index, e.reserved);
 			goto fail;
 		}
-		if (kvm_set_msr(vcpu, e.index, e.value)) {
-			pr_debug_ratelimited(
-				"%s cannot write MSR (%u, 0x%x, 0x%llx)\n",
-				__func__, i, e.index, e.value);
+		if (SET_MSR_OR_WARN(vcpu, e.index, e.value))
 			goto fail;
-		}
 	}
 	return 0;
 fail:

arch/x86/kvm/vmx/nested.c static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu) · line 4174

Quoted 8 lines
arch/x86/kvm/vmx/nested.c +1 -4
@@ -4175,12 +4171,8 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu)
 				goto vmabort;
 			}
 
-			if (kvm_set_msr(vcpu, h.index, h.value)) {
-				pr_debug_ratelimited(
-					"%s WRMSR failed (%u, 0x%x, 0x%llx)\n",
-					__func__, j, h.index, h.value);
+			if (SET_MSR_OR_WARN(vcpu, h.index, h.value))

A few comments on commit 458151f65b4d, which I obviously waited to long too review :-)

I find the WARN part of SET_MSR_OR_WARN() to be misleading. For me, WARN means exactly that, an actual on WARN or WARN_ON, which has different implications, especially when running with KERN_PANIC_ON_WARN.

I also don't like incorporating SET_MSR in the macro as it's not obvious what action will be taken without looking at the macro itself. IMO the code is more readable if only the print is macrofied, e.g.:

			if (kvm_set_msr(vcpu, h.index, h.value)
				NVMX_LOG_WRMSR_ERROR(h.index, h.value);

As for the original code, arguably it *should* do a full WARN and not simply log the error, as kvm_set_msr() should never fail if VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL was exposed to L1, unlike the above two cases where KVM is processing an L1-controlled MSR list, e.g.:

	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
		WARN_ON_ONCE(kvm_set_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL,
					 vmcs12->host_ia32_perf_global_ctrl));

Back to this patch, this isn't simply consolidating code, it's promoting L1-controlled messages from pr_debug() to pr_warn().

What if you add a patch to remove SET_MSR_OR_WARN() and instead manually do the WARN_ON_ONCE() as above, and then introduce a new macro to consolidate the pr_debug_ratelimited() stuff in this patch?

Quoted 8 lines
arch/x86/kvm/vmx/nested.c +0 -1
 				goto vmabort;
-			}
 		}
 	}
Signature · 1 line
2.24.0.432.g9d3f5f5b63-goog
maintainer
#2 lore raw
Quoted 9 lines · Sean Christopherson

As for the original code, arguably it *should* do a full WARN and not simply log the error, as kvm_set_msr() should never fail if VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL was exposed to L1, unlike the above two cases where KVM is processing an L1-controlled MSR list, e.g.:

	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
		WARN_ON_ONCE(kvm_set_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL,
					 vmcs12->host_ia32_perf_global_ctrl));
Quoted 6 lines

Back to this patch, this isn't simply consolidating code, it's promoting L1-controlled messages from pr_debug() to pr_warn().

What if you add a patch to remove SET_MSR_OR_WARN() and instead manually do the WARN_ON_ONCE() as above, and then introduce a new macro to consolidate the pr_debug_ratelimited() stuff in this patch?

Should go without saying (Sean is a Certified Reviewer according to MAINTAINERS :)) but I agree.

Paolo

author
#3 lore raw
Quoted 10 lines · Paolo Bonzini
Quoted 9 lines · Sean Christopherson

As for the original code, arguably it *should* do a full WARN and not simply log the error, as kvm_set_msr() should never fail if VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL was exposed to L1, unlike the above two cases where KVM is processing an L1-controlled MSR list, e.g.:

	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
		WARN_ON_ONCE(kvm_set_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL,
					 vmcs12->host_ia32_perf_global_ctrl));
Quoted 6 lines
Quoted 6 lines

Back to this patch, this isn't simply consolidating code, it's promoting L1-controlled messages from pr_debug() to pr_warn().

What if you add a patch to remove SET_MSR_OR_WARN() and instead manually do the WARN_ON_ONCE() as above, and then introduce a new macro to consolidate the pr_debug_ratelimited() stuff in this patch?

Sean,

Thank you for the detailed review of this patch (as well as the last one that I snuck past you :-P). I'm in complete agreement with your sentiments, a follow-up is in order. I'll get that out soon.

Quoted 4 lines

Should go without saying (Sean is a Certified Reviewer according to MAINTAINERS :)) but I agree.

Paolo

Sean has been a great help in providing detailed reviews -- well deserving of the designation! Thank you for pinging this thread, Paolo.

--
Best,
Oliver