Skip to contentSkip to conversation

call vmx_load_host_state() only if msr is cached

PATCH In review Gleb Natapov 2 participants 3 messages Aug 27, 2009 – Aug 31, 2009

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

Gleb Natapov

author
#0 lore raw

PATCH 1/1 call vmx_load_host_state() only if msr is cached

No need to call it before each kvm_(set|get)_msr_common()

Signed-off-by Gleb Natapov <[email protected]>
arch/x86/kvm/vmx.c +2 -2
index 6b57eed..7494ef1 100644
@@ -1005,9 +1005,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
 		data = vmcs_readl(GUEST_SYSENTER_ESP);
 		break;
 	default:
-		vmx_load_host_state(to_vmx(vcpu));
 		msr = find_msr_entry(to_vmx(vcpu), msr_index);
 		if (msr) {
+			vmx_load_host_state(to_vmx(vcpu));
 			data = msr->data;
 			break;
 		}

@@ -1064,9 +1064,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
 		}
 		/* Otherwise falls through to kvm_set_msr_common */
 	default:
-		vmx_load_host_state(vmx);
 		msr = find_msr_entry(vmx, msr_index);
 		if (msr) {
+			vmx_load_host_state(vmx);
 			msr->data = data;
 			break;
 		}

--
Gleb.

#1 lore raw
Quoted 19 lines · Gleb Natapov

No need to call it before each kvm_(set|get)_msr_common()

Signed-off-by Gleb Natapov <[email protected]>
arch/x86/kvm/vmx.c +1 -1
index 6b57eed..7494ef1 100644
@@ -1005,9 +1005,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
 		data = vmcs_readl(GUEST_SYSENTER_ESP);
 		break;
 	default:
-		vmx_load_host_state(to_vmx(vcpu));
 		msr = find_msr_entry(to_vmx(vcpu), msr_index);
 		if (msr) {
+			vmx_load_host_state(to_vmx(vcpu));
 			data = msr->data;
 			break;
 		}

arch/x86/kvm/vmx.c static int vmx_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data) · line 1069

Quoted 7 lines
arch/x86/kvm/vmx.c +1 -1
@@ -1064,9 +1064,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
 		}
 		/* Otherwise falls through to kvm_set_msr_common */
 	default:
-		vmx_load_host_state(vmx);
 		msr = find_msr_entry(vmx, msr_index);
 		if (msr) {
+			vmx_load_host_state(vmx);

All is needed here save_msrs(vmx->guest_msrs), right?

Looks fine to me. Do you have any numbers or just notice it out of code review?

Quoted 9 lines
arch/x86/kvm/vmx.c +0 -1
 			msr->data = data;
 			break;
 		}
--

Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#2 lore raw
Quoted 21 lines · Marcelo Tosatti
Quoted 20 lines · Gleb Natapov

No need to call it before each kvm_(set|get)_msr_common()

Signed-off-by Gleb Natapov <[email protected]>
arch/x86/kvm/vmx.c +1 -1
index 6b57eed..7494ef1 100644
@@ -1005,9 +1005,9 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
 		data = vmcs_readl(GUEST_SYSENTER_ESP);
 		break;
 	default:
-		vmx_load_host_state(to_vmx(vcpu));
 		msr = find_msr_entry(to_vmx(vcpu), msr_index);
 		if (msr) {
+			vmx_load_host_state(to_vmx(vcpu));
 			data = msr->data;
 			break;
 		}

@@ -1064,9 +1064,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
 		}
Quoted 9 lines

arch/x86/kvm/vmx.c static int vmx_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data) · line 1069

Quoted 6 lines
arch/x86/kvm/vmx.c +1 -1
 		/* Otherwise falls through to kvm_set_msr_common */
 	default:
-		vmx_load_host_state(vmx);
 		msr = find_msr_entry(vmx, msr_index);
 		if (msr) {
+			vmx_load_host_state(vmx);

All is needed here save_msrs(vmx->guest_msrs), right?

Yes. Otherwise we return wrong value for cached MSR.

Quoted 2 lines

Looks fine to me. Do you have any numbers or just notice it out of code review?

We saw this while profiling IPI ping-pong with x2apic (which uses MSR). This patch improves numbers by ~5us IIRC.

--
Gleb.