diff mbox series

[v4,1/9] KVM: nVMX: Use kvm_set_msr to load IA32_PERF_GLOBAL_CTRL on vmexit

Message ID 20190906210313.128316-2-oupton@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: VMX: Add full nested support for IA32_PERF_GLOBAL_CTRL | expand

Commit Message

Oliver Upton Sept. 6, 2019, 9:03 p.m. UTC 
The existing implementation for loading the IA32_PERF_GLOBAL_CTRL MSR
on VM-exit was incorrect, as the next call to atomic_switch_perf_msrs()
could cause this value to be overwritten. Instead, call kvm_set_msr()
which will allow atomic_switch_perf_msrs() to correctly set the values.

Suggested-by: Jim Mattson <jmattson@google.com>
Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Signed-off-by: Oliver Upton <oupton@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Peter Shier <pshier@google.com>
---
 arch/x86/kvm/vmx/nested.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Sean Christopherson Sept. 16, 2019, 6:03 p.m. UTC | #1 
On Fri, Sep 06, 2019 at 02:03:05PM -0700, Oliver Upton wrote:
> The existing implementation for loading the IA32_PERF_GLOBAL_CTRL MSR
> on VM-exit was incorrect, as the next call to atomic_switch_perf_msrs()
> could cause this value to be overwritten. Instead, call kvm_set_msr()
> which will allow atomic_switch_perf_msrs() to correctly set the values.
> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Peter Shier <pshier@google.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index ced9fba32598..b0ca34bf4d21 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -3724,6 +3724,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>  				   struct vmcs12 *vmcs12)
>  {
>  	struct kvm_segment seg;
> +	struct msr_data msr_info;
>  	u32 entry_failure_code;
>  
>  	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
> @@ -3800,9 +3801,15 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>  		vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
>  		vcpu->arch.pat = vmcs12->host_ia32_pat;
>  	}
> -	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
> -		vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
> -			vmcs12->host_ia32_perf_global_ctrl);
> +	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL) {
> +		msr_info.host_initiated = false;
> +		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
> +		msr_info.data = vmcs12->host_ia32_perf_global_ctrl;
> +		if (kvm_set_msr(vcpu, &msr_info))
> +			pr_debug_ratelimited(
> +				"%s cannot write MSR (0x%x, 0x%llx)\n",
> +				__func__, msr_info.index, msr_info.data);

Since this is for a single MSR, we should print the human-readable name of
the MSR as well as its index, e.g.:

			pr_debug_ratelimited(
				"%s cannot write PERF_GLOBAL_CONTROL MSR (0x%x, 0x%llx)\n",

> +	}
>  
>  	/* Set L1 segment info according to Intel SDM
>  	    27.5.2 Loading Host Segment and Descriptor-Table Registers */
> -- 
> 2.23.0.187.g17f5b7556c-goog
>
Sean Christopherson Sept. 16, 2019, 6:15 p.m. UTC | #2 
On Fri, Sep 06, 2019 at 02:03:05PM -0700, Oliver Upton wrote:
> The existing implementation for loading the IA32_PERF_GLOBAL_CTRL MSR
> on VM-exit was incorrect, as the next call to atomic_switch_perf_msrs()
> could cause this value to be overwritten. Instead, call kvm_set_msr()
> which will allow atomic_switch_perf_msrs() to correctly set the values.
> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Co-developed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Peter Shier <pshier@google.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index ced9fba32598..b0ca34bf4d21 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -3724,6 +3724,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>  				   struct vmcs12 *vmcs12)
>  {
>  	struct kvm_segment seg;
> +	struct msr_data msr_info;
>  	u32 entry_failure_code;
>  
>  	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
> @@ -3800,9 +3801,15 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
>  		vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
>  		vcpu->arch.pat = vmcs12->host_ia32_pat;
>  	}
> -	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
> -		vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
> -			vmcs12->host_ia32_perf_global_ctrl);
> +	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL) {
> +		msr_info.host_initiated = false;
> +		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
> +		msr_info.data = vmcs12->host_ia32_perf_global_ctrl;
> +		if (kvm_set_msr(vcpu, &msr_info))
> +			pr_debug_ratelimited(
> +				"%s cannot write MSR (0x%x, 0x%llx)\n",
> +				__func__, msr_info.index, msr_info.data);

I belatedly realized we don't actually do anything on failure.  If you
reorder the series to add the checks (currently patches 4/9 and 5/9) at
the very beginning, can we then WARN on failure here (and in the guest
flow too)?

> +	}
>  
>  	/* Set L1 segment info according to Intel SDM
>  	    27.5.2 Loading Host Segment and Descriptor-Table Registers */
> -- 
> 2.23.0.187.g17f5b7556c-goog
>
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index ced9fba32598..b0ca34bf4d21 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3724,6 +3724,7 @@  static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
 				   struct vmcs12 *vmcs12)
 {
 	struct kvm_segment seg;
+	struct msr_data msr_info;
 	u32 entry_failure_code;
 
 	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER)
@@ -3800,9 +3801,15 @@  static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
 		vmcs_write64(GUEST_IA32_PAT, vmcs12->host_ia32_pat);
 		vcpu->arch.pat = vmcs12->host_ia32_pat;
 	}
-	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)
-		vmcs_write64(GUEST_IA32_PERF_GLOBAL_CTRL,
-			vmcs12->host_ia32_perf_global_ctrl);
+	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL) {
+		msr_info.host_initiated = false;
+		msr_info.index = MSR_CORE_PERF_GLOBAL_CTRL;
+		msr_info.data = vmcs12->host_ia32_perf_global_ctrl;
+		if (kvm_set_msr(vcpu, &msr_info))
+			pr_debug_ratelimited(
+				"%s cannot write MSR (0x%x, 0x%llx)\n",
+				__func__, msr_info.index, msr_info.data);
+	}
 
 	/* Set L1 segment info according to Intel SDM
 	    27.5.2 Loading Host Segment and Descriptor-Table Registers */