KVM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] KVM: nVMX: Use SET_MSR_OR_WARN() to simplify failure logging
@ 2019-11-28  9:46 Oliver Upton
  2019-12-02 21:21 ` Sean Christopherson
  0 siblings, 1 reply; 3+ messages in thread
From: Oliver Upton @ 2019-11-28  9:46 UTC (permalink / raw)
  To: kvm; +Cc: Paolo Bonzini, Radim Krčmář, 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 <oupton@google.com>
---
 arch/x86/kvm/vmx/nested.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 4aea7d304beb..f7dbaac7cb90 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -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;
-			}
 		}
 	}
 
-- 
2.24.0.432.g9d3f5f5b63-goog


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] KVM: nVMX: Use SET_MSR_OR_WARN() to simplify failure logging
  2019-11-28  9:46 [PATCH] KVM: nVMX: Use SET_MSR_OR_WARN() to simplify failure logging Oliver Upton
@ 2019-12-02 21:21 ` Sean Christopherson
  2019-12-12  0:43   ` Paolo Bonzini
  0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2019-12-02 21:21 UTC (permalink / raw)
  To: Oliver Upton; +Cc: kvm, Paolo Bonzini, Radim Krčmář

On Thu, Nov 28, 2019 at 01:46:09AM -0800, Oliver Upton wrote:
> 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 <oupton@google.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 4aea7d304beb..f7dbaac7cb90 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -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))

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?

>  				goto vmabort;
> -			}
>  		}
>  	}
>  
> -- 
> 2.24.0.432.g9d3f5f5b63-goog
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] KVM: nVMX: Use SET_MSR_OR_WARN() to simplify failure logging
  2019-12-02 21:21 ` Sean Christopherson
@ 2019-12-12  0:43   ` Paolo Bonzini
  0 siblings, 0 replies; 3+ messages in thread
From: Paolo Bonzini @ 2019-12-12  0:43 UTC (permalink / raw)
  To: Sean Christopherson, Oliver Upton; +Cc: kvm, Radim Krčmář

On 02/12/19 22:21, Sean Christopherson wrote:
> 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?

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

Paolo


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-28  9:46 [PATCH] KVM: nVMX: Use SET_MSR_OR_WARN() to simplify failure logging Oliver Upton
2019-12-02 21:21 ` Sean Christopherson
2019-12-12  0:43   ` Paolo Bonzini

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git